Maint: parc.find_regions as module method. clarify find_regions
This PR addresses the following issues/points (see):
parcellation.find_regions()was a static method ofParcellationbut 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 usingfindto search within the parcellation. (findis a method ofRegion).- There is also
atlas.find_regions()as an instance method. It behaves differently thanparcellation.find_regions(). Needs to be clarified. - Finally, instead of using caching with a dictionary with keys of kwarg tuples, why not use
cacheproperty?lru_cachemight be necessary for long-running systems like siibra-api
(Please see the discussions on the code changes for further details)
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 :warning: |
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
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
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
findin 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.
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_regionsfor 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.