search

FZJ-INM1-BDA / siibra-python

Software interfaces for interacting with brain atlases - Python client
Apache License 2.0
46 stars 8 forks source link

feat: update how related regions are fetched #582

Closed xgui3783 closed 2 months ago

xgui3783 commented 3 months ago

see https://github.com/FZJ-INM1-BDA/siibra-configurations/pull/44

This PR is the companion PR for updating the SANDS reference within siibra.

Rather than relying on SANDS references of parcellation entity version, it is updated to use parcellation entity, which semantically more closely models SANDS model.

n.b. as this PR requires a specific branch of siibra configuration to function properly, this PR also aims to introduce the mechanism, by which, based on commit message, custom ref of siibra-configuration will be used.

These changes are not yet pushed, in order to follow the Red/Green light approach for TDD

codecov-commenter commented 3 months ago

Codecov Report

Attention: Patch coverage is 28.88889% with 32 lines in your changes are missing coverage. Please review.

Project coverage is 49.50%. Comparing base (5a0e6b3) to head (e1cff22). Report is 477 commits behind head on main.

Files Patch % Lines
siibra/core/region.py 28.88% 32 Missing :warning:

:exclamation: Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #582 +/- ## =========================================== + Coverage 36.81% 49.50% +12.68% =========================================== Files 61 74 +13 Lines 5421 6961 +1540 =========================================== + Hits 1996 3446 +1450 - Misses 3425 3515 +90 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

xgui3783 commented 3 months ago

Expecting e2e tests to fail (specifically e2e/core/test_regions.py)

xgui3783 commented 3 months ago

Tests should pass.

xgui3783 commented 3 months ago

@AhmetNSimsek can you please take a look at this?

I have also managed to pull in the changes that allow a custom ref of siibra-configuration to be used with [ci:usecfg]<refname>

xgui3783 commented 2 months ago

@AhmetNSimsek can you take a look? I added docstring as you requested.

xgui3783 commented 2 months ago

Thanks, looks good.

For future reference, would you mind using numpy style as in

https://github.com/FZJ-INM1-BDA/siibra-python/blob/50d4d4c3db1f185a0666cd793127748c6a904cb3/siibra/core/region.py#L273-L297

This should also work but better to have a unified style. Please don't waste time on it now tho. There are more urgent things atm.

Apologies, I have a lot on my plate right now.

Do you mind taking over the reformatting for me?

@AhmetNSimsek

AhmetNSimsek commented 2 months ago

Since the config branch had been merged and tagged, bumped the version (3b2d9fa) to test with the correct config.