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

[ENH]: Introduce get_template for getting templates #298

Merged
synchon merged 10 commits from feat/get-template into main 2024-02-09 10:56:45 +00:00
Conversation 21 Commits 10 Files changed 7 +122 -5
synchon commented 2024-01-31 10:12:51 +00:00 (Migrated from github.com)
Copy link
  • description of feature/fix
  • tests added/passed
  • add an entry for the latest changes

This PR introduces a new function junifer.data.get_template() to get template space images via templateflow, tailored to a target data.

* [x] description of feature/fix * [x] tests added/passed * [x] add an entry for the latest changes This PR introduces a new function `junifer.data.get_template()` to get template space images via `templateflow`, tailored to a target data.
codecov[bot] commented 2024-01-31 10:13:46 +00:00 (Migrated from github.com)
Copy link

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (4397f3f) 89.16% compared to head (1d5d635) 89.15%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #298      +/-   ##
==========================================
- Coverage   89.16%   89.15%   -0.01%     
==========================================
  Files         101      101              
  Lines        4466     4483      +17     
  Branches      854      856       +2     
==========================================
+ Hits         3982     3997      +15     
- Misses        346      348       +2     
  Partials      138      138
Flag Coverage Δ
junifer 89.15% <89.47%> (-0.01%) ⬇️

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

Files Coverage Δ
junifer/data/__init__.py 100.00% <100.00%> (ø)
junifer/data/utils.py 100.00% <ø> (ø)
junifer/data/template_spaces.py 90.47% <88.88%> (-9.53%) ⬇️
## [Codecov](https://app.codecov.io/gh/juaml/junifer/pull/298?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml) Report Attention: `2 lines` in your changes are missing coverage. Please review. > Comparison is base [(`4397f3f`)](https://app.codecov.io/gh/juaml/junifer/commit/4397f3f5d096c7980972bf6cfe4c0c62e78559c2?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml) 89.16% compared to head [(`1d5d635`)](https://app.codecov.io/gh/juaml/junifer/pull/298?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml) 89.15%. <details><summary>Additional details and impacted files</summary> [![Impacted file tree graph](https://app.codecov.io/gh/juaml/junifer/pull/298/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://app.codecov.io/gh/juaml/junifer/pull/298?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml) ```diff @@ Coverage Diff @@ ## main #298 +/- ## ========================================== - Coverage 89.16% 89.15% -0.01% ========================================== Files 101 101 Lines 4466 4483 +17 Branches 854 856 +2 ========================================== + Hits 3982 3997 +15 - Misses 346 348 +2 Partials 138 138 ``` | [Flag](https://app.codecov.io/gh/juaml/junifer/pull/298/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml) | Coverage Δ | | |---|---|---| | [junifer](https://app.codecov.io/gh/juaml/junifer/pull/298/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml) | `89.15% <89.47%> (-0.01%)` | :arrow_down: | 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. | [Files](https://app.codecov.io/gh/juaml/junifer/pull/298?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://app.codecov.io/gh/juaml/junifer/pull/298?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml#diff-anVuaWZlci9kYXRhL19faW5pdF9fLnB5) | `100.00% <100.00%> (ø)` | | | [junifer/data/utils.py](https://app.codecov.io/gh/juaml/junifer/pull/298?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml#diff-anVuaWZlci9kYXRhL3V0aWxzLnB5) | `100.00% <ø> (ø)` | | | [junifer/data/template\_spaces.py](https://app.codecov.io/gh/juaml/junifer/pull/298?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml#diff-anVuaWZlci9kYXRhL3RlbXBsYXRlX3NwYWNlcy5weQ==) | `90.47% <88.88%> (-9.53%)` | :arrow_down: | </details>
synchon commented 2024-01-31 10:17:57 +00:00 (Migrated from github.com)
Copy link

Should be merged after #297

Should be merged after #297
github-actions[bot] commented 2024-01-31 10:19:47 +00:00 (Migrated from github.com)
Copy link
PR Preview Action v1.4.7
Preview removed because the pull request was closed.
2024-02-09 11:00 UTC
[PR Preview Action](https://github.com/rossjrw/pr-preview-action) v1.4.7 :---: Preview removed because the pull request was closed. 2024-02-09 11:00 UTC <!-- Sticky Pull Request Commentpr-preview -->
fraimondo (Migrated from github.com) requested changes 2024-02-07 13:01:02 +00:00
junifer/data/template_spaces.py
@ -92,0 +160,4 @@
klass=RuntimeError,
)
else:
return nib.load(template_path) # type: ignore
fraimondo (Migrated from github.com) commented 2024-02-07 13:00:55 +00:00
Copy link

We need to get the "closest resolution" here

We need to get the "closest resolution" here
synchon (Migrated from github.com) reviewed 2024-02-07 13:07:17 +00:00
junifer/data/template_spaces.py
@ -92,0 +160,4 @@
klass=RuntimeError,
)
else:
return nib.load(template_path) # type: ignore
synchon (Migrated from github.com) commented 2024-02-07 13:07:17 +00:00
Copy link

The closest_resolution function takes a list of valid resolutions, but we don't have that info right away. For that we need an extra scraping of the templateflow metadata which adds a bit of overhead. I don't get the complete rationale, maybe I can implement it in a different way if I understand correctly.

The `closest_resolution` function takes a list of valid resolutions, but we don't have that info right away. For that we need an extra scraping of the templateflow metadata which adds a bit of overhead. I don't get the complete rationale, maybe I can implement it in a different way if I understand correctly.
fraimondo (Migrated from github.com) reviewed 2024-02-07 13:34:51 +00:00
junifer/data/template_spaces.py
@ -92,0 +160,4 @@
klass=RuntimeError,
)
else:
return nib.load(template_path) # type: ignore
fraimondo (Migrated from github.com) commented 2024-02-07 13:34:51 +00:00
Copy link

The idea of that function is to tell you which is the resolution of the parcellation/mask/etc that you need to use for the image you have.

The idea of that function is to tell you which is the resolution of the parcellation/mask/etc that you need to use for the image you have.
synchon (Migrated from github.com) reviewed 2024-02-07 13:37:58 +00:00
junifer/data/template_spaces.py
@ -92,0 +160,4 @@
klass=RuntimeError,
)
else:
return nib.load(template_path) # type: ignore
synchon (Migrated from github.com) commented 2024-02-07 13:37:58 +00:00
Copy link

We are already getting the resolution for the template required from the target data and it needs to be int for templateflow. If templateflow doesn't have that, it throws an error. So in that case, we need to resample.

Also closest_resolution works for parcellations and masks as we already know the available resolutions for them.

We are already getting the resolution for the template required from the target data and it needs to be int for templateflow. If templateflow doesn't have that, it throws an error. So in that case, we need to resample. Also `closest_resolution` works for parcellations and masks as we already know the available resolutions for them.
fraimondo (Migrated from github.com) reviewed 2024-02-07 13:39:18 +00:00
junifer/data/template_spaces.py
@ -92,0 +160,4 @@
klass=RuntimeError,
)
else:
return nib.load(template_path) # type: ignore
fraimondo (Migrated from github.com) commented 2024-02-07 13:39:18 +00:00
Copy link

Exactly, basically the closest_resolution function will tell you which one to get in case you don't have the resolution of your image

Exactly, basically the `closest_resolution` function will tell you which one to get in case you don't have the resolution of your image
synchon (Migrated from github.com) reviewed 2024-02-07 13:40:20 +00:00
junifer/data/template_spaces.py
@ -92,0 +160,4 @@
klass=RuntimeError,
)
else:
return nib.load(template_path) # type: ignore
synchon (Migrated from github.com) commented 2024-02-07 13:40:19 +00:00
Copy link

But in our case, we have it and we fetch accordingly.

But in our case, we have it and we fetch accordingly.
fraimondo (Migrated from github.com) reviewed 2024-02-07 13:45:58 +00:00
junifer/data/template_spaces.py
@ -92,0 +160,4 @@
klass=RuntimeError,
)
else:
return nib.load(template_path) # type: ignore
fraimondo (Migrated from github.com) commented 2024-02-07 13:45:58 +00:00
Copy link

no, we don't have "any" resolution. We have some resolutions. Templateflow is the same. We have A, B, C and D as resolutions. Let's say our image is in resolution E. We need to get the "closest" to E following the criteria defined in the "closest resolution" function.

no, we don't have "any" resolution. We have some resolutions. Templateflow is the same. We have A, B, C and D as resolutions. Let's say our image is in resolution E. We need to get the "closest" to E following the criteria defined in the "closest resolution" function.
synchon (Migrated from github.com) reviewed 2024-02-07 13:49:04 +00:00
junifer/data/template_spaces.py
@ -92,0 +160,4 @@
klass=RuntimeError,
)
else:
return nib.load(template_path) # type: ignore
synchon (Migrated from github.com) commented 2024-02-07 13:49:04 +00:00
Copy link

So there are two options:

  • scrape templateflow metadata to get updated info but has overhead
  • define the possible resolutions for all templateflow-provided templates
So there are two options: - scrape templateflow metadata to get updated info but has overhead - define the possible resolutions for all templateflow-provided templates
synchon (Migrated from github.com) reviewed 2024-02-07 13:52:09 +00:00
junifer/data/template_spaces.py
@ -92,0 +160,4 @@
klass=RuntimeError,
)
else:
return nib.load(template_path) # type: ignore
synchon (Migrated from github.com) commented 2024-02-07 13:52:09 +00:00
Copy link

Just to put it here, closest_resolution is used in functions like load_parcellation and load_mask where one can choose the resolution, but in functions like get_parcellation and get_mask, we use the target's resolution to resample the data obtained via load_* functions.

Just to put it here, `closest_resolution` is used in functions like `load_parcellation` and `load_mask` where one can choose the resolution, but in functions like `get_parcellation` and `get_mask`, we use the target's resolution to resample the data obtained via `load_*` functions.
fraimondo (Migrated from github.com) reviewed 2024-02-07 13:53:54 +00:00
junifer/data/template_spaces.py
@ -92,0 +160,4 @@
klass=RuntimeError,
)
else:
return nib.load(template_path) # type: ignore
fraimondo (Migrated from github.com) commented 2024-02-07 13:53:54 +00:00
Copy link

Junifer needs to able to run without network connections. My take is to scrape on the first time and save in the junifer_data directory (if we follow the scrape option).

My issue with hard-coding the resolutions is what might happen if templateflow adds a new resolution. We might need to update the code.

Junifer needs to able to run without network connections. My take is to scrape on the first time and save in the `junifer_data` directory (if we follow the scrape option). My issue with hard-coding the resolutions is what might happen if templateflow adds a new resolution. We might need to update the code.
synchon (Migrated from github.com) reviewed 2024-02-07 14:09:07 +00:00
junifer/data/template_spaces.py
@ -92,0 +160,4 @@
klass=RuntimeError,
)
else:
return nib.load(template_path) # type: ignore
synchon (Migrated from github.com) commented 2024-02-07 14:09:07 +00:00
Copy link

Templateflow does the caching so should be able to work without network connection. To make this clean and work in a global way, it's better to have something like a junifer precache command which downloads required data for datagrabber, parcellations, masks and template spaces.

Templateflow does the caching so should be able to work without network connection. To make this clean and work in a global way, it's better to have something like a `junifer precache` command which downloads required data for datagrabber, parcellations, masks and template spaces.
fraimondo (Migrated from github.com) reviewed 2024-02-07 14:58:19 +00:00
junifer/data/template_spaces.py
@ -92,0 +160,4 @@
klass=RuntimeError,
)
else:
return nib.load(template_path) # type: ignore
fraimondo (Migrated from github.com) commented 2024-02-07 14:58:18 +00:00
Copy link

100% agree with the precache. I though of calling it prefetch. And that's why everything that is donwloaded (like parcellations) are stored in the junifer_data directory, to make it explicit and permanent across nodes/sessions/users.

In this case, check which of the above solutions works best in your opinion.

100% agree with the `precache`. I though of calling it `prefetch`. And that's why everything that is donwloaded (like parcellations) are stored in the `junifer_data` directory, to make it explicit and permanent across nodes/sessions/users. In this case, check which of the above solutions works best in your opinion.
synchon (Migrated from github.com) reviewed 2024-02-07 15:00:39 +00:00
junifer/data/template_spaces.py
@ -92,0 +160,4 @@
klass=RuntimeError,
)
else:
return nib.load(template_path) # type: ignore
synchon (Migrated from github.com) commented 2024-02-07 15:00:39 +00:00
Copy link

I would want to make a separate PR tackling this specific feature and come back to get_template then. What do you think?

I would want to make a separate PR tackling this specific feature and come back to `get_template` then. What do you think?
fraimondo (Migrated from github.com) reviewed 2024-02-09 08:28:17 +00:00
junifer/data/template_spaces.py
@ -92,0 +160,4 @@
klass=RuntimeError,
)
else:
return nib.load(template_path) # type: ignore
fraimondo (Migrated from github.com) commented 2024-02-09 08:28:17 +00:00
Copy link

Do you mean precache? Yes, that's a totally separate PR.

Do you mean `precache`? Yes, that's a totally separate PR.
synchon (Migrated from github.com) reviewed 2024-02-09 08:30:21 +00:00
junifer/data/template_spaces.py
@ -92,0 +160,4 @@
klass=RuntimeError,
)
else:
return nib.load(template_path) # type: ignore
synchon (Migrated from github.com) commented 2024-02-09 08:30:21 +00:00
Copy link

Yes.

For this PR, the scraping won't be optimal until we have precache.

Yes. For this PR, the scraping won't be optimal until we have precache.
fraimondo (Migrated from github.com) reviewed 2024-02-09 08:32:13 +00:00
junifer/data/template_spaces.py
@ -92,0 +160,4 @@
klass=RuntimeError,
)
else:
return nib.load(template_path) # type: ignore
fraimondo (Migrated from github.com) commented 2024-02-09 08:32:13 +00:00
Copy link

First we make it work, then we optimise.

First we make it work, then we optimise.
synchon (Migrated from github.com) reviewed 2024-02-09 09:33:28 +00:00
junifer/data/template_spaces.py
@ -92,0 +160,4 @@
klass=RuntimeError,
)
else:
return nib.load(template_path) # type: ignore
synchon (Migrated from github.com) commented 2024-02-09 09:33:28 +00:00
Copy link

The latest commits should address your concern.

The latest commits should address your concern.
fraimondo (Migrated from github.com) approved these changes 2024-02-09 09:36:06 +00:00
Sign in to join this conversation.
Reviewers
No reviewers
fraimondo
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
0.0.4 (alpha 3)
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!298
No description provided.