Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Render roi thumbnail fixes #157

Merged
merged 4 commits into from
Jun 16, 2020

Conversation

will-moore
Copy link
Member

@will-moore will-moore commented Mar 25, 2020

Found bug in render_roi_thumbnail as part of the iviewer work at ome/omero-iviewer#311
We don't use render_roi_thumbnail in any of our clients (old webgateway viewer uses Shapes thumbnails, not ROIs).

Added tests at ome/openmicroscopy#6235

To test:

  • For an ROI with a single shape where Z & T are not set, try /webgateway/render_roi_thumbnail/{ROI_ID}/

@will-moore will-moore closed this Mar 27, 2020
@will-moore will-moore reopened this Mar 27, 2020
@pwalczysko
Copy link
Member

Tested 4 cases

See links , user-3

All works as expected, I am getting insets such as in the screenshot below.

Ready to merge fmpov.

Screen Shot 2020-03-31 at 11 46 40

@joshmoore
Copy link
Member

Sorry, @will-moore. Can you describe in words what the change in behavior is?

@will-moore
Copy link
Member Author

Previously I got an exception if theT was None:

File "/Users/wmoore/Desktop/WEB/omero-web/omeroweb/webgateway/views.py" in render_roi_thumbnail
  436.             if t < minT:

Exception Type: TypeError at /webgateway/render_roi_thumbnail/175180
Exception Value: '<' not supported between instances of 'NoneType' and 'NoneType'

@joshmoore
Copy link
Member

Understood and thanks. Like the public user fix, would be good to see this rolled into a release soon. Only possible follow-up thought is if there's would be any chance in our test suite to add a check for this.

@will-moore
Copy link
Member Author

Added tests at ome/openmicroscopy#6235

@jburel
Copy link
Member

jburel commented Jun 11, 2020

Few more BytesIO in the method that are not closed e.g.

rv = BytesIO()
img = Image.open(BytesIO(jpeg_data))

and few others in render_image, render_shape_masks. The one in the last 2 methods could be fixed in a follow-up PR if you prefer.

@will-moore
Copy link
Member Author

I would prefer not to add more to this PR.
Already this and ome/openmicroscopy#6235 have grown from the original bug fix (which isn't needed now for ome/omero-iviewer#311) so it would be good to get them merged.

@jburel
Copy link
Member

jburel commented Jun 15, 2020

Could you create an issue so we review the resources opened in a follow-up PR?

@will-moore
Copy link
Member Author

Created #175

@will-moore
Copy link
Member Author

will-moore commented Jun 16, 2020

Tests failing at https://merge-ci.openmicroscopy.org/jenkins/job/OMERO-test-integration/427/testReport/junit/OmeroWeb.test.integration.test_thumbnails/TestRoiThumbnails/test_roi_thumbnail_0_1_/

midZ = zList[len(zList)/2]
E       TypeError: list indices must be integers or slices, not float

because this PR is NOT merged and ome/openmicroscopy#6235 is merged.

@will-moore will-moore closed this Jun 16, 2020
@will-moore will-moore reopened this Jun 16, 2020
@joshmoore
Copy link
Member

Retriggered the integration tests now that the commit is green.

@joshmoore joshmoore merged commit c87f9f6 into ome:master Jun 16, 2020
@sbesson sbesson added this to the 5.7.0 milestone Jun 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants