-
Notifications
You must be signed in to change notification settings - Fork 10
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
Conversation
f0427d2
to
827db31
Compare
Codecov ReportAttention: Patch coverage is
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. |
There was a problem hiding this 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.
Just to explain the original logic - there were different entry points for different scopes:
I think the logic is clear, but I understand that the automatic exposure of the class method |
Thank you. Then, IMO, we should make |
…, region.species does not need to check parcellation's existence
This PR addresses the following issues/points (see):
parcellation.find_regions()
was a static method ofParcellation
but it searches through all parcellations in the registry. For the user, when they have a parcellation instanceparc
, there is no clear difference betweenparc.find_regions()
andparc.find()
while they should be usingfind
to search within the parcellation. (find
is a method ofRegion
).atlas.find_regions()
as an instance method. It behaves differently thanparcellation.find_regions()
. Needs to be clarified.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