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

add getROIs #380

Merged
merged 5 commits into from
Aug 23, 2023
Merged

add getROIs #380

merged 5 commits into from
Aug 23, 2023

Conversation

barrettMCW
Copy link
Contributor

hey y'all! I was going to put a patch into our groups dev library but i figured I'd see if you wanted it. Since getROICount already gets the ROIs i just put the get part in a different method and made count just check the length of results.

@will-moore
Copy link
Member

Hi, and thanks for the suggestion.

Methods of the BlitzGateway (e.g. getROIs()) would normally be expected to return BlitzObjectWrapper objects, not e.g. omero.model.Roi but rather wrap the roi with omero.gateway.RoiWrapper(conn, roi).

However, you wouldn't want to init all those objects when just counting ROIs as there's potential for BlitzObjectWrapper to load more data on init.
So you might want a def _get_rois() to return omero.model.roi objects as you currently have for getROIs(), use this for getROICount() and then wrap those ROIs to return Wrapper objects for def getROIs().
Does that make sense?

@barrettMCW
Copy link
Contributor Author

Good stuff, thanks! getROIs and getROICount now use a private function _get_rois to get valid roi model objects, and getROIs wraps those and returns them.

@will-moore
Copy link
Member

Just added the --include label, since I noticed this wasn't being merged in the daily build: see https://merge-ci.openmicroscopy.org/jenkins/job/OMERO-python-superbuild-push/271/console

Then we can confirm tests are still passing at https://merge-ci.openmicroscopy.org/jenkins/job/OMERO-test-integration/lastCompletedBuild/testReport/OmeroPy.test.integration.test_rois/

In due course, it would also be nice to add tests for conn.getROIs() to https://github.com/ome/openmicroscopy/blob/develop/components/tools/OmeroPy/test/integration/test_rois.py

filterByCurrentUser is True, otherwise the total rois
found.
"""
return [RoiWrapper(self.conn, roi) for roi in
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs to be self._conn (missing underscore).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry about that and the delay to fix, been OOO. should be good now. I wouldn't mind writing up a little test in the openmicroscopy repo if you guys want me to. thanks y'all!

@joshmoore joshmoore merged commit 7c2f644 into ome:master Aug 23, 2023
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants