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

Maint: parc.find_regions as module method. clarify find_regions #517

Merged
merged 7 commits into from
Dec 13, 2024

Conversation

AhmetNSimsek
Copy link
Collaborator

@AhmetNSimsek AhmetNSimsek commented Nov 29, 2023

This PR addresses the following issues/points (see):

  • parcellation.find_regions() was a static method of Parcellation but it searches through all parcellations in the registry. For the user, when they have a parcellation instance parc, there is no clear difference between parc.find_regions() and parc.find() while they should be using find to search within the parcellation. (find is a method of Region).
  • There is also atlas.find_regions() as an instance method. It behaves differently than parcellation.find_regions(). Needs to be clarified.
  • Finally, instead of using caching with a dictionary with keys of kwarg tuples, why not use cache property?
    • lru_cache might be necessary for long-running systems like siibra-api

(Please see the discussions on the code changes for further details)

Fixes #513

@codecov-commenter
Copy link

codecov-commenter commented Nov 29, 2023

Codecov Report

Attention: Patch coverage is 94.11765% with 1 line in your changes missing coverage. Please review.

Project coverage is 45.61%. Comparing base (4d21343) to head (5e4b460).
Report is 13 commits behind head on main.

Files with missing lines Patch % Lines
siibra/features/anchor.py 50.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #517      +/-   ##
==========================================
- Coverage   45.64%   45.61%   -0.03%     
==========================================
  Files          75       75              
  Lines        7184     7184              
==========================================
- Hits         3279     3277       -2     
- Misses       3905     3907       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@xgui3783 xgui3783 left a comment

Choose a reason for hiding this comment

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

Mostly fine.

I have questions where find_region should still be in parcellation.py

and the aggressive caching strategy we are using everywhere.

siibra/core/parcellation.py Outdated Show resolved Hide resolved
siibra/core/parcellation.py Show resolved Hide resolved
siibra/VERSION Outdated Show resolved Hide resolved
siibra/core/parcellation.py Outdated Show resolved Hide resolved
@AhmetNSimsek AhmetNSimsek added the maintenance Not a bug or breaking issue. Code maintenance related. label Dec 7, 2023
@dickscheid
Copy link
Contributor

dickscheid commented Feb 21, 2024

Just to explain the original logic - there were different entry points for different scopes:

  • search any region known to siibra was dessigned as a class method Parcellation.find_regions(), searching through all instances of parcellations at runtime (maybe it was implemented as static which I agree was not proper)
  • search only the regions linked to a species was implemented as an instance method of Atlas, which holds the parcellations of that species
  • search regions in one particular parcellation only was implemented as an instance method of the parcellation, actually implemented as find in Region, since region is the parent class which holds the subtree and allows for a recursive implementation.

I think the logic is clear, but I understand that the automatic exposure of the class method Parcellation.find_regions for Parcellation instances leads to confusion.

@AhmetNSimsek
Copy link
Collaborator Author

Just to explain the original logic - there were different entry points for different scopes:

* search any region known to siibra was dessigned as a class method `Parcellation.find_regions()`, searching through all instances of parcellations at runtime (maybe it was implemented as static which I agree was not proper)

* search only the regions linked to a species was implemented as an instance method of Atlas, which holds the parcellations of that species

* search regions in one particular parcellation only was implemented as an instance method of the parcellation, actually implemented as  `find` in Region, since region is the parent class which holds the subtree and allows for a recursive implementation.

I think the logic is clear, but I understand that the automatic exposure of the class method Parcellation.find_regions for Parcellation instances leads to confusion.

Thank you. Then, IMO, we should make find_regions a module level method in parcellation.py or if it is static or class method, it should be hidden and only be forwarded with siibra.find_regions. I'd prefer the first as I do not see any reason for such a method to be class or instance method.

siibra/core/region.py Outdated Show resolved Hide resolved
siibra/core/region.py Outdated Show resolved Hide resolved
@AhmetNSimsek AhmetNSimsek merged commit a679b28 into main Dec 13, 2024
30 of 41 checks passed
@AhmetNSimsek AhmetNSimsek deleted the maint_find_regions branch December 13, 2024 13:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintenance Not a bug or breaking issue. Code maintenance related.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Placement of Parcellation.find_regions (and Atlas.find_regions) and it's functionality requires clarification
4 participants