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 Facility for 'shallow' loading of objects #68

Merged
merged 6 commits into from
Mar 17, 2023

Conversation

dominikl
Copy link
Member

The Java gateway uses the ContainerService for loading Projects, Datasets, etc. However against IDR this becomes very slow, see #67 .

This PR adds an new Facility for loading these 'container' objects in a 'shallow' way via the QueryService. Only one level of hierarchy is loaded (e.g. a project will have a list of datasets, but they won't have any images loaded).

@dominikl dominikl requested a review from jburel November 18, 2022 10:12
@jburel
Copy link
Member

jburel commented Dec 6, 2022

I cannot see any tests for the PR.

@@ -187,7 +188,7 @@ public Map<Long, Long> getAnnotationsCounts() {
*
* @return See above.
*/
public Set getImages() {
public Set<ImageData> getImages() {
Copy link
Member

Choose a reason for hiding this comment

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

This could break using the method

Copy link
Member Author

Choose a reason for hiding this comment

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

Is there a non-breaking way to fix that? Although I'm not sure if it really would break client code... I'll test. I'll also add integration tests for the new class.

Copy link
Member

Choose a reason for hiding this comment

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

when adding new tests, you will need to disable the GHA otherwise it will be red.

Copy link
Member Author

Choose a reason for hiding this comment

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

I tested that with a method which returns a Collection, and changed it to return a Collection<String>, and it doesn't break code using the method. You'll only get a warning in the IDE that the cast which had to be used before the change is now redundant. I think this change is safe to get in.

Copy link
Member

Choose a reason for hiding this comment

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

thanks for checking

@dominikl dominikl force-pushed the load_objects_light branch from 681d86b to 54e0de2 Compare March 14, 2023 11:52
@dominikl
Copy link
Member Author

Integration tests: ome/openmicroscopy#6342
Tests are green now, i.e. would be ok to merge before gateway release @jburel ?

Set<WellData> wells = new HashSet();
for (Well w : asPlate().copyWells())
wells.add(new WellData(w));
return wells;
Copy link
Member

Choose a reason for hiding this comment

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

other methods returning list return null for empty collections. This does not match since an empty list will be returned if the plate has no wells

Copy link
Member Author

Choose a reason for hiding this comment

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

Indeed, thanks 👍

@dominikl dominikl requested a review from jburel March 16, 2023 09:27
Copy link
Member

@jburel jburel left a comment

Choose a reason for hiding this comment

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

Thanks
I will prep a release and include it in the upcoming openmicroscopy release

@jburel jburel merged commit 30ca945 into ome:master Mar 17, 2023
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.

2 participants