juaml/junifer
Watch
3
0
Fork
You've already forked junifer
 0
Code Issues 14 Pull requests 2 Packages Activity Actions

Feat/expose parc merge #202

Merged
LeSasse merged 8 commits from feat/expose_parc_merge into main 2023-03-27 14:55:15 +00:00
Conversation 18 Commits 8 Files changed 6 +270 -115
LeSasse commented 2023-03-25 06:07:40 +00:00 (Migrated from github.com)
Copy link
  • fix #146
  • description of feature/fix
  • tests added/passed
  • add an entry to the latest changes

This exposes the merging of parcellations (for example cortical and subcortical parcellations, i.e. schaefer + tian parcellations) as a function for the user, so that they can have merged parcellations available for subsequent analyses.

* [x] fix #146 * [x] description of feature/fix * [x] tests added/passed * [x] add an entry to the [latest changes](../docs/changes/latest.inc) This exposes the merging of parcellations (for example cortical and subcortical parcellations, i.e. schaefer + tian parcellations) as a function for the user, so that they can have merged parcellations available for subsequent analyses.
codecov[bot] commented 2023-03-25 06:09:02 +00:00 (Migrated from github.com)
Copy link

Codecov Report

Merging #202 (4d549f9) into main (1d3c721) will decrease coverage by 0.03%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #202      +/-   ##
==========================================
- Coverage   93.23%   93.21%   -0.03%     
==========================================
  Files          80       80              
  Lines        3385     3360      -25     
  Branches      627      620       -7     
==========================================
- Hits         3156     3132      -24     
+ Misses        151      150       -1     
  Partials       78       78
Flag Coverage Δ
docs 100.00% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
junifer/data/__init__.py 100.00% <ø> (ø)
junifer/data/parcellations.py 97.82% <ø> (-0.03%) ⬇️
junifer/datagrabber/datalad_base.py 91.26% <ø> (+0.71%) ⬆️
junifer/markers/parcel_aggregation.py 100.00% <ø> (ø)
## [Codecov](https://codecov.io/gh/juaml/junifer/pull/202?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml) Report > Merging [#202](https://codecov.io/gh/juaml/junifer/pull/202?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml) (4d549f9) into [main](https://codecov.io/gh/juaml/junifer/commit/1d3c7216f6063a8f5d7b2d3b4917c0845716a4a6?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml) (1d3c721) will **decrease** coverage by `0.03%`. > The diff coverage is `n/a`. [![Impacted file tree graph](https://codecov.io/gh/juaml/junifer/pull/202/graphs/tree.svg?width=650&height=150&src=pr&token=5H21JuZXMw&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml)](https://codecov.io/gh/juaml/junifer/pull/202?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml) ```diff @@ Coverage Diff @@ ## main #202 +/- ## ========================================== - Coverage 93.23% 93.21% -0.03% ========================================== Files 80 80 Lines 3385 3360 -25 Branches 627 620 -7 ========================================== - Hits 3156 3132 -24 + Misses 151 150 -1 Partials 78 78 ``` | Flag | Coverage Δ | | |---|---|---| | docs | `100.00% <ø> (ø)` | | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml#carryforward-flags-in-the-pull-request-comment) to find out more. | [Impacted Files](https://codecov.io/gh/juaml/junifer/pull/202?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml) | Coverage Δ | | |---|---|---| | [junifer/data/\_\_init\_\_.py](https://codecov.io/gh/juaml/junifer/pull/202?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml#diff-anVuaWZlci9kYXRhL19faW5pdF9fLnB5) | `100.00% <ø> (ø)` | | | [junifer/data/parcellations.py](https://codecov.io/gh/juaml/junifer/pull/202?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml#diff-anVuaWZlci9kYXRhL3BhcmNlbGxhdGlvbnMucHk=) | `97.82% <ø> (-0.03%)` | :arrow_down: | | [junifer/datagrabber/datalad\_base.py](https://codecov.io/gh/juaml/junifer/pull/202?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml#diff-anVuaWZlci9kYXRhZ3JhYmJlci9kYXRhbGFkX2Jhc2UucHk=) | `91.26% <ø> (+0.71%)` | :arrow_up: | | [junifer/markers/parcel\_aggregation.py](https://codecov.io/gh/juaml/junifer/pull/202?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml#diff-anVuaWZlci9tYXJrZXJzL3BhcmNlbF9hZ2dyZWdhdGlvbi5weQ==) | `100.00% <ø> (ø)` | |
github-actions[bot] commented 2023-03-25 07:49:37 +00:00 (Migrated from github.com)
Copy link
PR Preview Action v1.3.0
Preview removed because the pull request was closed.
2023-03-27 14:59 UTC
[PR Preview Action](https://github.com/rossjrw/pr-preview-action) v1.3.0 :---: Preview removed because the pull request was closed. 2023-03-27 14:59 UTC <!-- Sticky Pull Request Commentpr-preview -->
fraimondo (Migrated from github.com) requested changes 2023-03-27 07:57:18 +00:00
fraimondo (Migrated from github.com) left a comment
Copy link

@LeSasse :

Can you copy this tests and adapt them to the merge_parcellations test?

github.com/juaml/junifer@af2a8cf999/junifer/markers/tests/test_parcel_aggregation.py (L293)

They are basically testing overlapping / duplicated labels and stuff like that. This should be also tested here as it is no longer the responsibility of the marker to deal with several parcellations.

@LeSasse : Can you copy this tests and adapt them to the `merge_parcellations` test? https://github.com/juaml/junifer/blob/af2a8cf999949624859594d7f8522035ff02ac78/junifer/markers/tests/test_parcel_aggregation.py#L293 They are basically testing overlapping / duplicated labels and stuff like that. This should be also tested here as it is no longer the responsibility of the marker to deal with several parcellations.
fraimondo commented 2023-03-27 07:57:44 +00:00 (Migrated from github.com)
Copy link

Rest is perfect, just need the proper tests.

Rest is perfect, just need the proper tests.
synchon (Migrated from github.com) requested changes 2023-03-27 08:53:45 +00:00
docs/changes/latest.inc
synchon (Migrated from github.com) commented 2023-03-27 08:44:46 +00:00
Copy link

Would be good to have the function reference here.

Would be good to have the function reference here.
junifer/data/parcellations.py
@ -700,3 +700,84 @@ def _retrieve_suit(
].to_list()
synchon (Migrated from github.com) commented 2023-03-27 08:45:32 +00:00
Copy link

Is it List[str]?

Is it `List[str]`?
synchon (Migrated from github.com) commented 2023-03-27 08:51:38 +00:00
Copy link

I think a bit of explicit types for List and Tuple would be nice.

I think a bit of explicit types for `List` and `Tuple` would be nice.
synchon (Migrated from github.com) commented 2023-03-27 08:52:23 +00:00
Copy link

The basic types like str can be documented here as well for ease.

The basic types like str can be documented here as well for ease.
LeSasse (Migrated from github.com) reviewed 2023-03-27 10:41:21 +00:00
docs/changes/latest.inc
LeSasse (Migrated from github.com) commented 2023-03-27 10:41:20 +00:00
Copy link

@LeSasse :

Can you copy this tests and adapt them to the merge_parcellations test?

github.com/juaml/junifer@af2a8cf999/junifer/markers/tests/test_parcel_aggregation.py (L293)

They are basically testing overlapping / duplicated labels and stuff like that. This should be also tested here as it is no longer the responsibility of the marker to deal with several parcellations.

ok, will check it out!

> @LeSasse : > > Can you copy this tests and adapt them to the `merge_parcellations` test? > > https://github.com/juaml/junifer/blob/af2a8cf999949624859594d7f8522035ff02ac78/junifer/markers/tests/test_parcel_aggregation.py#L293 > > They are basically testing overlapping / duplicated labels and stuff like that. This should be also tested here as it is no longer the responsibility of the marker to deal with several parcellations. ok, will check it out!
LeSasse (Migrated from github.com) reviewed 2023-03-27 10:42:09 +00:00
junifer/data/parcellations.py
@ -700,3 +700,84 @@ def _retrieve_suit(
].to_list()
LeSasse (Migrated from github.com) commented 2023-03-27 10:42:09 +00:00
Copy link

no, it should be List["Nifti1Image"] i think, should i put it?

no, it should be `List["Nifti1Image"]` i think, should i put it?
LeSasse (Migrated from github.com) reviewed 2023-03-27 10:45:26 +00:00
junifer/data/parcellations.py
@ -700,3 +700,84 @@ def _retrieve_suit(
].to_list()
LeSasse (Migrated from github.com) commented 2023-03-27 10:45:26 +00:00
Copy link

makes sense, for the Tuple it works as Tuple["Nifti1Image", List[str]]?

makes sense, for the Tuple it works as `Tuple["Nifti1Image", List[str]]`?
synchon (Migrated from github.com) reviewed 2023-03-27 10:45:36 +00:00
junifer/data/parcellations.py
@ -700,3 +700,84 @@ def _retrieve_suit(
].to_list()
synchon (Migrated from github.com) commented 2023-03-27 10:45:36 +00:00
Copy link

Yeah.

Yeah.
LeSasse (Migrated from github.com) reviewed 2023-03-27 10:46:05 +00:00
junifer/data/parcellations.py
@ -700,3 +700,84 @@ def _retrieve_suit(
].to_list()
LeSasse (Migrated from github.com) commented 2023-03-27 10:46:05 +00:00
Copy link

you mean parcellations_list : list of Nifti1Image?

you mean `parcellations_list : list of Nifti1Image`?
synchon (Migrated from github.com) reviewed 2023-03-27 10:46:16 +00:00
junifer/data/parcellations.py
@ -700,3 +700,84 @@ def _retrieve_suit(
].to_list()
synchon (Migrated from github.com) commented 2023-03-27 10:46:16 +00:00
Copy link

Yeah exactly.

Yeah exactly.
synchon (Migrated from github.com) reviewed 2023-03-27 10:47:47 +00:00
junifer/data/parcellations.py
@ -700,3 +700,84 @@ def _retrieve_suit(
].to_list()
synchon (Migrated from github.com) commented 2023-03-27 10:47:47 +00:00
Copy link

Yeah but I think it should be ... niimg-like object as nibabel calls it.

Yeah but I think it should be `... niimg-like object` as `nibabel` calls it.
LeSasse (Migrated from github.com) reviewed 2023-03-27 10:50:13 +00:00
docs/changes/latest.inc
LeSasse (Migrated from github.com) commented 2023-03-27 10:50:12 +00:00
Copy link

By copy, you mean move them over to the test_merge_parcellations tests only, such that no multiple parcellation scenario is tested in test_parcel_aggregation anymore @fraimondo?

By copy, you mean move them over to the `test_merge_parcellations` tests only, such that no multiple parcellation scenario is tested in `test_parcel_aggregation` anymore @fraimondo?
LeSasse (Migrated from github.com) reviewed 2023-03-27 10:51:48 +00:00
junifer/data/parcellations.py
@ -700,3 +700,84 @@ def _retrieve_suit(
].to_list()
LeSasse (Migrated from github.com) commented 2023-03-27 10:51:47 +00:00
Copy link

ok will try that, i think i put niimg before which the docs didn't recognise, but niimg-like object makes sense

ok will try that, i think i put `niimg` before which the docs didn't recognise, but `niimg-like object` makes sense
fraimondo (Migrated from github.com) reviewed 2023-03-27 12:06:30 +00:00
synchon (Migrated from github.com) reviewed 2023-03-27 12:09:01 +00:00
fraimondo (Migrated from github.com) approved these changes 2023-03-27 14:51:07 +00:00
synchon (Migrated from github.com) approved these changes 2023-03-27 14:52:24 +00:00
Sign in to join this conversation.
Reviewers
No reviewers
fraimondo
synchon
Labels
Clear labels   
CRITICAL
High Priority Issue

  
Stale

   
WIP
Work in progress

  
bug
Something isn't working

  
concept
Design/Implementation concepts

  
coordinate
Issues or pull requests related to coordinates (ROIs)

  
dataset
Issues or pull requests related to datagrabbers or datasets

  
dependencies
Pull requests that update a dependency file

  
documentation
Improvements or additions to documentation

  
duplicate
This issue or pull request already exists

  
enhancement
New feature or request

  
github_actions
Pull requests that update GitHub Actions code

  
good first issue
Good for newcomers

  
help wanted
Extra attention is needed

  
invalid
This doesn't seem right

  
maintenance
Issues or pull requests related to maintenance

  
maps
Issues or pull requests related to maps (probabilistic atlases)

  
marker
Issues or pull requests related to markers

  
mask
Issues or pull requests related to masks

  
on hold
Issue waiting on other issue(s)

  
parcellation
Issues or pull requests related to parcellations (deterministic atlases)

  
preprocess
Issues or pull requests related to preprocessors

  
question
Further information is requested

  
ready
Pull request is ready for review and merge

  
storage
Issues or pull requests related to marker storage or retrieval

  
template-space
Issues or pull requests related to template spaces

  
triage
New issues waiting to be reviewed

  
wontfix
This will not be worked on

No labels
CRITICAL
Stale
WIP
bug
concept
coordinate
dataset
dependencies
documentation
duplicate
enhancement
github_actions
good first issue
help wanted
invalid
maintenance
maps
marker
mask
on hold
parcellation
preprocess
question
ready
storage
template-space
triage
wontfix
Milestone
Clear milestone
No items
No milestone
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference
juaml/junifer!202
No description provided.