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

[ENH]: Add support for subject(s) file for junifer run #182

Merged
synchon merged 19 commits from update/allow-element-via-file into main 2023-10-10 13:04:45 +00:00
Conversation 12 Commits 19 Files changed 8 +334 -23
synchon commented 2023-08-24 17:20:41 +00:00 (Migrated from github.com)
Copy link

Are you requiring a new dataset or marker?

  • I understand this is not a marker or dataset request

Which feature do you want to include?

I think in most data processing projects it is desirable to be able to select specific subsets of all subjects. Sometimes one just wants to get all available data, but sometimes one may want only specific subsets (like for example only unrelated subjects or subjects matched on some other variable). In other cases one may want to just preprocess a specific subset for some initial exploration or testing before getting the full sample.

How do you imagine this integrated in junifer?

Add a subject parameter to in-built data grabbers. Ideally in the full pipeline one can add a list of subjects to the yaml directly, or for example by providing a "subject.txt" file that lists all desired subjects. Something like that.

Do you have a sample code that implements this outside of junifer?

No response

Anything else to say?

No response

### Are you requiring a new dataset or marker? - [X] I understand this is not a marker or dataset request ### Which feature do you want to include? I think in most data processing projects it is desirable to be able to select specific subsets of all subjects. Sometimes one just wants to get all available data, but sometimes one may want only specific subsets (like for example only unrelated subjects or subjects matched on some other variable). In other cases one may want to just preprocess a specific subset for some initial exploration or testing before getting the full sample. ### How do you imagine this integrated in junifer? Add a subject parameter to in-built data grabbers. Ideally in the full pipeline one can add a list of subjects to the yaml directly, or for example by providing a "subject.txt" file that lists all desired subjects. Something like that. ### Do you have a sample code that implements this outside of junifer? _No response_ ### Anything else to say? _No response_
fraimondo commented 2023-03-31 08:00:19 +00:00 (Migrated from github.com)
Copy link

This was thought already: github.com/juaml/junifer@ace98da340/junifer/api/cli.py (L54)

The only thing we need to define is the file format.

  • CSV?
  • TSV?
  • both depending on the extension?
  • With headers?
  • without headers?

Main issue is elements as tuples.

For example, using HCP1200 datagrabber, we can set in the datagrabber task="REST1" and then the iterator of the datagrabber will go through all the element tuples, but only for the specified task:

(sub-000001, REST1, LR)
(sub-000001, REST1, RL)
(sub-000002, REST1, LR)
(sub-000002, REST1, RL)
...

Setting the elements on the CLI or the YAML is just a way of bypassing the iterator. So in this case, the .CSV file with the elements to process should contain all the keys of the element ("subject", "task", "phase_encoding").

Possible solutions:

  • Modify the datagrabber so we can also get an iterator using a list, in which some of the keys of the element can be left out. Something like a filter function:
def filter(self, selection) -> Iterator:

then these lines in the run function:
github.com/juaml/junifer@ace98da340/junifer/api/functions.py (L165-L172)

would change for something like:

 with datagrabber_object: 
     if elements is not None: 
         for t_element in datagrabber_object.filter(elements): 
             mc.fit(datagrabber_object[t_element]) 
     else: 
         for t_element in datagrabber_object: 
             mc.fit(datagrabber_object[t_element])
  • Ask the user to provide the list all elements with all keys in it.
This was thought already: https://github.com/juaml/junifer/blob/ace98da340e20cffcd2a5d939ff0d57afe4451d0/junifer/api/cli.py#L54 The only thing we need to define is the file format. * CSV? * TSV? * both depending on the extension? * With headers? * without headers? Main issue is *elements* as tuples. For example, using HCP1200 datagrabber, we can set in the datagrabber `task="REST1"` and then the iterator of the datagrabber will go through all the element tuples, but only for the specified task: ``` (sub-000001, REST1, LR) (sub-000001, REST1, RL) (sub-000002, REST1, LR) (sub-000002, REST1, RL) ... ``` Setting the elements on the CLI or the YAML is just a way of bypassing the iterator. So in this case, the .CSV file with the elements to process should contain all the keys of the element (`"subject"`, `"task"`, `"phase_encoding"`). Possible solutions: * Modify the datagrabber so we can also get an iterator using a list, in which some of the keys of the element can be left out. Something like a `filter` function: ``` def filter(self, selection) -> Iterator: ``` then these lines in the run function: https://github.com/juaml/junifer/blob/ace98da340e20cffcd2a5d939ff0d57afe4451d0/junifer/api/functions.py#L165-L172 would change for something like: ```python with datagrabber_object: if elements is not None: for t_element in datagrabber_object.filter(elements): mc.fit(datagrabber_object[t_element]) else: for t_element in datagrabber_object: mc.fit(datagrabber_object[t_element]) ``` * Ask the user to provide the list all elements with all keys in it.
LeSasse commented 2023-03-31 08:16:07 +00:00 (Migrated from github.com)
Copy link

What you say makes sense, but I really only mean like setting one parameter of the element, i.e. mostly for subjects, to a list. The rest of the element can still be constructed. So for example, as a user I dont want to construct all elements, but just provide a list of subjects, for example of the HCP in a txt file. Junifer can then construct the elements, for example by using the default values for all other parameters (i.e. [LR, RL] and [REST1..., LAST_TASK]), without me having to make an iterator/file over all elements myself. The reasoning is that subject lists can be quite long and are therefore more difficult to set in the YAML file than other parameters. Having said that, making a csv file of all elements is also easy enough, but in my opinion puts more burden on the user (also in terms of data discovery).

What you say makes sense, but I really only mean like setting one parameter of the element, i.e. mostly for subjects, to a list. The rest of the element can still be constructed. So for example, as a user I dont want to construct all elements, but just provide a list of subjects, for example of the HCP in a txt file. Junifer can then construct the elements, for example by using the default values for all other parameters (i.e. [LR, RL] and [REST1..., LAST_TASK]), without me having to make an iterator/file over all elements myself. The reasoning is that subject lists can be quite long and are therefore more difficult to set in the YAML file than other parameters. Having said that, making a csv file of all elements is also easy enough, but in my opinion puts more burden on the user (also in terms of data discovery).
synchon commented 2023-07-19 04:51:52 +00:00 (Migrated from github.com)
Copy link

Passing only the subject IDs makes sense to me as this will make it simple for users and also enable one to filter what the DataGrabber iterates over in combination with other parameters that one DataGrabber provides, for example,types, tasks and so on. From the implementation perspective, we already have a good way as @fraimondo shows.

Passing only the subject IDs makes sense to me as this will make it simple for users and also enable one to filter what the `DataGrabber` iterates over in combination with other parameters that one `DataGrabber` provides, for example,`types`, `tasks` and so on. From the implementation perspective, we already have a good way as @fraimondo shows.
fraimondo commented 2023-07-19 06:50:35 +00:00 (Migrated from github.com)
Copy link

Then let's move on that way.

Then let's move on that way.
👍 1
github-actions[bot] commented 2023-08-24 17:26:57 +00:00 (Migrated from github.com)
Copy link
PR Preview Action v1.4.4
Preview removed because the pull request was closed.
2023-10-10 13:10 UTC
[PR Preview Action](https://github.com/rossjrw/pr-preview-action) v1.4.4 :---: Preview removed because the pull request was closed. 2023-10-10 13:10 UTC <!-- Sticky Pull Request Commentpr-preview -->
codecov[bot] commented 2023-09-06 11:45:56 +00:00 (Migrated from github.com)
Copy link

Codecov Report

Merging #182 (8666562) into main (22ca06f) will increase coverage by 0.01%.
Report is 3 commits behind head on main.
The diff coverage is 96.42%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #182      +/-   ##
==========================================
+ Coverage   93.03%   93.04%   +0.01%     
==========================================
  Files          84       84              
  Lines        3718     3739      +21     
  Branches      724      733       +9     
==========================================
+ Hits         3459     3479      +20     
  Misses        161      161              
- Partials       98       99       +1
Flag Coverage Δ
docs 100.00% <ø> (ø)
junifer 93.04% <96.42%> (+0.01%) ⬆️

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

Files Coverage Δ
junifer/api/cli.py 71.28% <100.00%> (+2.13%) ⬆️
junifer/api/functions.py 96.23% <100.00%> (ø)
junifer/datagrabber/base.py 98.36% <93.75%> (-1.64%) ⬇️
## [Codecov](https://app.codecov.io/gh/juaml/junifer/pull/182?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml) Report > Merging [#182](https://app.codecov.io/gh/juaml/junifer/pull/182?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml) (8666562) into [main](https://app.codecov.io/gh/juaml/junifer/commit/22ca06fe0872b83749ccc1de2917b549be4ffef4?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml) (22ca06f) will **increase** coverage by `0.01%`. > Report is 3 commits behind head on main. > The diff coverage is `96.42%`. [![Impacted file tree graph](https://app.codecov.io/gh/juaml/junifer/pull/182/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/182?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml) ```diff @@ Coverage Diff @@ ## main #182 +/- ## ========================================== + Coverage 93.03% 93.04% +0.01% ========================================== Files 84 84 Lines 3718 3739 +21 Branches 724 733 +9 ========================================== + Hits 3459 3479 +20 Misses 161 161 - Partials 98 99 +1 ``` | [Flag](https://app.codecov.io/gh/juaml/junifer/pull/182/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml) | Coverage Δ | | |---|---|---| | [docs](https://app.codecov.io/gh/juaml/junifer/pull/182/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml) | `100.00% <ø> (ø)` | | | [junifer](https://app.codecov.io/gh/juaml/junifer/pull/182/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml) | `93.04% <96.42%> (+0.01%)` | :arrow_up: | 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/182?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml) | Coverage Δ | | |---|---|---| | [junifer/api/cli.py](https://app.codecov.io/gh/juaml/junifer/pull/182?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml#diff-anVuaWZlci9hcGkvY2xpLnB5) | `71.28% <100.00%> (+2.13%)` | :arrow_up: | | [junifer/api/functions.py](https://app.codecov.io/gh/juaml/junifer/pull/182?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml#diff-anVuaWZlci9hcGkvZnVuY3Rpb25zLnB5) | `96.23% <100.00%> (ø)` | | | [junifer/datagrabber/base.py](https://app.codecov.io/gh/juaml/junifer/pull/182?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml#diff-anVuaWZlci9kYXRhZ3JhYmJlci9iYXNlLnB5) | `98.36% <93.75%> (-1.64%)` | :arrow_down: |
fraimondo (Migrated from github.com) requested changes 2023-10-04 15:04:53 +00:00
junifer/api/cli.py
fraimondo (Migrated from github.com) commented 2023-10-04 15:01:41 +00:00
Copy link

While this works, it's not quite robust. Splitting elements by , should be done only if there's no file involved.

While this works, it's not quite robust. Splitting elements by `,` should be done only if there's no file involved.
junifer/api/cli.py
@ -61,19 +80,49 @@ def _parse_elements(element: str, config: Dict) -> Union[List, None]:
"over the configuration file. That is, the elements specified "
"in the command line will be used. The elements specified in "
"the configuration file will be ignored. To remove this warning, "
fraimondo (Migrated from github.com) commented 2023-10-04 15:03:22 +00:00
Copy link

This function should parse the file completely, giving the list of tuples required.

Why this?
Well, because we can have things like:
sub-01,ses-01 or sub-01, ses-01 or sub-01, ses-01 .

Using pandas to parse the CSV file would be easier.

This function should parse the file completely, giving the list of tuples required. Why this? Well, because we can have things like: `sub-01,ses-01` or `sub-01, ses-01` or `sub-01, ses-01 `. Using pandas to parse the CSV file would be easier.
synchon (Migrated from github.com) reviewed 2023-10-06 10:09:52 +00:00
junifer/api/cli.py
@ -61,19 +80,49 @@ def _parse_elements(element: str, config: Dict) -> Union[List, None]:
"over the configuration file. That is, the elements specified "
"in the command line will be used. The elements specified in "
"the configuration file will be ignored. To remove this warning, "
synchon (Migrated from github.com) commented 2023-10-06 10:09:52 +00:00
Copy link

Well, because we can have things like:
sub-01,ses-01 or sub-01, ses-01 or sub-01, ses-01 .

This worked but I anyway changed it to use pandas as you suggested.

> Well, because we can have things like: sub-01,ses-01 or sub-01, ses-01 or sub-01, ses-01 . This worked but I anyway changed it to use `pandas` as you suggested.
synchon (Migrated from github.com) reviewed 2023-10-06 10:10:04 +00:00
junifer/api/cli.py
synchon (Migrated from github.com) commented 2023-10-06 10:10:04 +00:00
Copy link

Addressed.

Addressed.
fraimondo (Migrated from github.com) requested changes 2023-10-10 09:16:48 +00:00
junifer/api/tests/test_cli.py
@ -77,0 +117,4 @@
contents["workdir"] = str(workdir.resolve())
# Output directory
outdir = tmp_path / "outdir"
# Storage
fraimondo (Migrated from github.com) commented 2023-10-10 09:16:34 +00:00
Copy link

This is not fully testing all the options.

What if the user wants to specify subject and session? Can you test that, it should be a comma-separated list.

This is not fully testing all the options. What if the user wants to specify `subject` and `session`? Can you test that, it should be a comma-separated list.
synchon (Migrated from github.com) reviewed 2023-10-10 11:03:47 +00:00
junifer/api/tests/test_cli.py
@ -77,0 +117,4 @@
contents["workdir"] = str(workdir.resolve())
# Output directory
outdir = tmp_path / "outdir"
# Storage
synchon (Migrated from github.com) commented 2023-10-10 11:03:47 +00:00
Copy link

Should be addressed now.

Should be addressed now.
fraimondo (Migrated from github.com) approved these changes 2023-10-10 13:03:50 +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!182
No description provided.