siibra-python icon indicating copy to clipboard operation
siibra-python copied to clipboard

Maint: parc.find_regions as module method. clarify find_regions

Open AhmetNSimsek opened this issue 2 years ago • 3 comments

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)

AhmetNSimsek avatar Nov 29 '23 10:11 AhmetNSimsek

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.

codecov-commenter avatar Nov 29 '23 14:11 codecov-commenter

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.

dickscheid avatar Feb 21 '24 11:02 dickscheid

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.

AhmetNSimsek avatar Feb 21 '24 11:02 AhmetNSimsek