[ENH]: Raise/warn if element is not ran #319

Merged
synchon merged 8 commits from enh/raise-invalid-element-selector into main 2025-01-08 17:29:31 +00:00
synchon commented 2024-12-18 17:22:01 +00:00 (Migrated from github.com)

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 just did this:

  patterns:
    VBM_GM: 
        pattern: derivatives/fmriprep/sub-{subject}/anat/sub-{subject}_space-MNI152NLin2009cAsym_label-GM_probseg.nii.gz
        space: MNI152NLin2009cAsym
  replacements: 
    - subject

and then tried to run junifer with --element sub-0001.

The issue (which take me a lot of time to solve) is that sub-0001 is not in the elements list, since the replacement is without the sub- prefix.

Now, there was no issue/warning from the run function. It cloned the dataset, did nothing and dropped the dataset.

How do you imagine this integrated in junifer?

We should implement a check in which if some of the elements in the list is not selected by the filter, there's a warning/error.

Exactly here is where we pass through the filter.

github.com/juaml/junifer@a9e799caa3/junifer/api/functions.py (L170-L178)

We should somehow keep a list of "unselected" elements so we can raise the warning.

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 just did this: ``` patterns: VBM_GM: pattern: derivatives/fmriprep/sub-{subject}/anat/sub-{subject}_space-MNI152NLin2009cAsym_label-GM_probseg.nii.gz space: MNI152NLin2009cAsym replacements: - subject ``` and then tried to run junifer with `--element sub-0001`. The issue (which take me a lot of time to solve) is that `sub-0001` is not in the _elements_ list, since the replacement is without the `sub-` prefix. Now, there was no issue/warning from the run function. It cloned the dataset, did nothing and dropped the dataset. ### How do you imagine this integrated in junifer? We should implement a check in which if some of the elements in the list is not selected by the filter, there's a warning/error. Exactly here is where we pass through the filter. https://github.com/juaml/junifer/blob/a9e799caa3433d3d4eaa633d4565771b8eb297f6/junifer/api/functions.py#L170-L178 We should somehow keep a list of "unselected" elements so we can raise the warning. ### Do you have a sample code that implements this outside of junifer? _No response_ ### Anything else to say? _No response_
synchon commented 2024-04-05 09:07:59 +00:00 (Migrated from github.com)

@fraimondo Do you need it for v0.0.4 or can we move it to v0.0.5?

@fraimondo Do you need it for v0.0.4 or can we move it to v0.0.5?
fraimondo commented 2024-04-05 09:34:47 +00:00 (Migrated from github.com)

It's not that easy to implement, mainly because element can be a part of the full tuple, to iterate over a subject's sessions.

However, what we can do for v0.0.4 is to raise a warning if len(elements) > len(processed_elements). We compute processed_elements just by counting how many times we fit the mc inside that for loop (Line 171)

It's not that easy to implement, mainly because `element` can be a part of the full tuple, to iterate over a subject's sessions. However, what we can do for v0.0.4 is to raise a warning if `len(elements) > len(processed_elements)`. We compute `processed_elements` just by counting how many times we fit the `mc` inside that for loop (Line 171)
synchon commented 2024-04-05 09:52:21 +00:00 (Migrated from github.com)

It's not that easy to implement, mainly because element can be a part of the full tuple, to iterate over a subject's sessions.

Yes that is a bit tricky.

However, what we can do for v0.0.4 is to raise a warning if len(elements) > len(processed_elements). We compute processed_elements just by counting how many times we fit the mc inside that for loop (Line 171)

Although the solution is good, I would like to have a comprehensive solution as it's not a critical feature issue, but more of a user notification.

> It's not that easy to implement, mainly because element can be a part of the full tuple, to iterate over a subject's sessions. Yes that is a bit tricky. > However, what we can do for v0.0.4 is to raise a warning if len(elements) > len(processed_elements). We compute processed_elements just by counting how many times we fit the mc inside that for loop (Line 171) Although the solution is good, I would like to have a comprehensive solution as it's not a critical feature issue, but more of a user notification.
github-actions[bot] commented 2024-12-18 18:18:42 +00:00 (Migrated from github.com)
PR Preview Action v1.5.0
Preview removed because the pull request was closed.
2025-01-08 18:23 UTC
[PR Preview Action](https://github.com/rossjrw/pr-preview-action) v1.5.0 :---: Preview removed because the pull request was closed. 2025-01-08 18:23 UTC <!-- Sticky Pull Request Commentpr-preview -->
codecov[bot] commented 2025-01-02 12:57:52 +00:00 (Migrated from github.com)

Codecov Report

Attention: Patch coverage is 0% with 29 lines in your changes missing coverage. Please review.

Project coverage is 0.01%. Comparing base (7e12e23) to head (fd25474).
Report is 9 commits behind head on main.

Files with missing lines Patch % Lines
junifer/api/functions.py 0.00% 8 Missing ⚠️
junifer/cli/parser.py 0.00% 5 Missing ⚠️
junifer/datagrabber/base.py 0.00% 5 Missing ⚠️
junifer/datagrabber/pattern.py 0.00% 3 Missing ⚠️
...er/api/queue_context/gnu_parallel_local_adapter.py 0.00% 2 Missing ⚠️
junifer/api/queue_context/htcondor_adapter.py 0.00% 2 Missing ⚠️
junifer/datagrabber/datalad_base.py 0.00% 2 Missing ⚠️
junifer/typing/_typing.py 0.00% 2 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff            @@
##            main    #319      +/-   ##
========================================
- Coverage   0.01%   0.01%   -0.01%     
========================================
  Files        133     133              
  Lines       5750    5764      +14     
========================================
  Hits           1       1              
- Misses      5749    5763      +14     
Flag Coverage Δ
docs 100.00% <ø> (ø)
junifer 0.00% <0.00%> (ø)

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

Files with missing lines Coverage Δ
...er/api/queue_context/gnu_parallel_local_adapter.py 0.00% <0.00%> (ø)
junifer/api/queue_context/htcondor_adapter.py 0.00% <0.00%> (ø)
junifer/datagrabber/datalad_base.py 0.00% <0.00%> (ø)
junifer/typing/_typing.py 0.00% <0.00%> (ø)
junifer/datagrabber/pattern.py 0.00% <0.00%> (ø)
junifer/cli/parser.py 0.00% <0.00%> (ø)
junifer/datagrabber/base.py 0.00% <0.00%> (ø)
junifer/api/functions.py 0.00% <0.00%> (ø)
## [Codecov](https://app.codecov.io/gh/juaml/junifer/pull/319?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml) Report Attention: Patch coverage is `0%` with `29 lines` in your changes missing coverage. Please review. > Project coverage is 0.01%. Comparing base [(`7e12e23`)](https://app.codecov.io/gh/juaml/junifer/commit/7e12e2357944a6850d2c7f36e3fbd0942a20b485?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml) to head [(`fd25474`)](https://app.codecov.io/gh/juaml/junifer/commit/fd254747f3462adee91646e10d3cd95580801c9a?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml). > Report is 9 commits behind head on main. | [Files with missing lines](https://app.codecov.io/gh/juaml/junifer/pull/319?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml) | Patch % | Lines | |---|---|---| | [junifer/api/functions.py](https://app.codecov.io/gh/juaml/junifer/pull/319?src=pr&el=tree&filepath=junifer%2Fapi%2Ffunctions.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml#diff-anVuaWZlci9hcGkvZnVuY3Rpb25zLnB5) | 0.00% | [8 Missing :warning: ](https://app.codecov.io/gh/juaml/junifer/pull/319?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml) | | [junifer/cli/parser.py](https://app.codecov.io/gh/juaml/junifer/pull/319?src=pr&el=tree&filepath=junifer%2Fcli%2Fparser.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml#diff-anVuaWZlci9jbGkvcGFyc2VyLnB5) | 0.00% | [5 Missing :warning: ](https://app.codecov.io/gh/juaml/junifer/pull/319?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml) | | [junifer/datagrabber/base.py](https://app.codecov.io/gh/juaml/junifer/pull/319?src=pr&el=tree&filepath=junifer%2Fdatagrabber%2Fbase.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml#diff-anVuaWZlci9kYXRhZ3JhYmJlci9iYXNlLnB5) | 0.00% | [5 Missing :warning: ](https://app.codecov.io/gh/juaml/junifer/pull/319?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml) | | [junifer/datagrabber/pattern.py](https://app.codecov.io/gh/juaml/junifer/pull/319?src=pr&el=tree&filepath=junifer%2Fdatagrabber%2Fpattern.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml#diff-anVuaWZlci9kYXRhZ3JhYmJlci9wYXR0ZXJuLnB5) | 0.00% | [3 Missing :warning: ](https://app.codecov.io/gh/juaml/junifer/pull/319?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml) | | [...er/api/queue\_context/gnu\_parallel\_local\_adapter.py](https://app.codecov.io/gh/juaml/junifer/pull/319?src=pr&el=tree&filepath=junifer%2Fapi%2Fqueue_context%2Fgnu_parallel_local_adapter.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml#diff-anVuaWZlci9hcGkvcXVldWVfY29udGV4dC9nbnVfcGFyYWxsZWxfbG9jYWxfYWRhcHRlci5weQ==) | 0.00% | [2 Missing :warning: ](https://app.codecov.io/gh/juaml/junifer/pull/319?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml) | | [junifer/api/queue\_context/htcondor\_adapter.py](https://app.codecov.io/gh/juaml/junifer/pull/319?src=pr&el=tree&filepath=junifer%2Fapi%2Fqueue_context%2Fhtcondor_adapter.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml#diff-anVuaWZlci9hcGkvcXVldWVfY29udGV4dC9odGNvbmRvcl9hZGFwdGVyLnB5) | 0.00% | [2 Missing :warning: ](https://app.codecov.io/gh/juaml/junifer/pull/319?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml) | | [junifer/datagrabber/datalad\_base.py](https://app.codecov.io/gh/juaml/junifer/pull/319?src=pr&el=tree&filepath=junifer%2Fdatagrabber%2Fdatalad_base.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml#diff-anVuaWZlci9kYXRhZ3JhYmJlci9kYXRhbGFkX2Jhc2UucHk=) | 0.00% | [2 Missing :warning: ](https://app.codecov.io/gh/juaml/junifer/pull/319?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml) | | [junifer/typing/\_typing.py](https://app.codecov.io/gh/juaml/junifer/pull/319?src=pr&el=tree&filepath=junifer%2Ftyping%2F_typing.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml#diff-anVuaWZlci90eXBpbmcvX3R5cGluZy5weQ==) | 0.00% | [2 Missing :warning: ](https://app.codecov.io/gh/juaml/junifer/pull/319?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml) | <details><summary>Additional details and impacted files</summary> [![Impacted file tree graph](https://app.codecov.io/gh/juaml/junifer/pull/319/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/319?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml) ```diff @@ Coverage Diff @@ ## main #319 +/- ## ======================================== - Coverage 0.01% 0.01% -0.01% ======================================== Files 133 133 Lines 5750 5764 +14 ======================================== Hits 1 1 - Misses 5749 5763 +14 ``` | [Flag](https://app.codecov.io/gh/juaml/junifer/pull/319/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/319/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/319/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml) | `0.00% <0.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. | [Files with missing lines](https://app.codecov.io/gh/juaml/junifer/pull/319?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml) | Coverage Δ | | |---|---|---| | [...er/api/queue\_context/gnu\_parallel\_local\_adapter.py](https://app.codecov.io/gh/juaml/junifer/pull/319?src=pr&el=tree&filepath=junifer%2Fapi%2Fqueue_context%2Fgnu_parallel_local_adapter.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml#diff-anVuaWZlci9hcGkvcXVldWVfY29udGV4dC9nbnVfcGFyYWxsZWxfbG9jYWxfYWRhcHRlci5weQ==) | `0.00% <0.00%> (ø)` | | | [junifer/api/queue\_context/htcondor\_adapter.py](https://app.codecov.io/gh/juaml/junifer/pull/319?src=pr&el=tree&filepath=junifer%2Fapi%2Fqueue_context%2Fhtcondor_adapter.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml#diff-anVuaWZlci9hcGkvcXVldWVfY29udGV4dC9odGNvbmRvcl9hZGFwdGVyLnB5) | `0.00% <0.00%> (ø)` | | | [junifer/datagrabber/datalad\_base.py](https://app.codecov.io/gh/juaml/junifer/pull/319?src=pr&el=tree&filepath=junifer%2Fdatagrabber%2Fdatalad_base.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml#diff-anVuaWZlci9kYXRhZ3JhYmJlci9kYXRhbGFkX2Jhc2UucHk=) | `0.00% <0.00%> (ø)` | | | [junifer/typing/\_typing.py](https://app.codecov.io/gh/juaml/junifer/pull/319?src=pr&el=tree&filepath=junifer%2Ftyping%2F_typing.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml#diff-anVuaWZlci90eXBpbmcvX3R5cGluZy5weQ==) | `0.00% <0.00%> (ø)` | | | [junifer/datagrabber/pattern.py](https://app.codecov.io/gh/juaml/junifer/pull/319?src=pr&el=tree&filepath=junifer%2Fdatagrabber%2Fpattern.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml#diff-anVuaWZlci9kYXRhZ3JhYmJlci9wYXR0ZXJuLnB5) | `0.00% <0.00%> (ø)` | | | [junifer/cli/parser.py](https://app.codecov.io/gh/juaml/junifer/pull/319?src=pr&el=tree&filepath=junifer%2Fcli%2Fparser.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml#diff-anVuaWZlci9jbGkvcGFyc2VyLnB5) | `0.00% <0.00%> (ø)` | | | [junifer/datagrabber/base.py](https://app.codecov.io/gh/juaml/junifer/pull/319?src=pr&el=tree&filepath=junifer%2Fdatagrabber%2Fbase.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml#diff-anVuaWZlci9kYXRhZ3JhYmJlci9iYXNlLnB5) | `0.00% <0.00%> (ø)` | | | [junifer/api/functions.py](https://app.codecov.io/gh/juaml/junifer/pull/319?src=pr&el=tree&filepath=junifer%2Fapi%2Ffunctions.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml#diff-anVuaWZlci9hcGkvZnVuY3Rpb25zLnB5) | `0.00% <0.00%> (ø)` | | </details>
fraimondo (Migrated from github.com) requested changes 2025-01-02 13:24:07 +00:00
@ -243,7 +263,7 @@ def queue(
kind: str,
jobname: str = "junifer_job",
fraimondo (Migrated from github.com) commented 2025-01-02 13:24:03 +00:00

This logic won't work in case element is part of a tuple of elements.

The idea is that we can have subject/session/task/etc as the element but only pass the "subject" part an the filter function will return all of the elements for that subject.

I think that the logic should be to check if all items in the element array yielded at least one valid element.

This logic won't work in case element is part of a tuple of elements. The idea is that we can have subject/session/task/etc as the element but only pass the "subject" part an the filter function will return all of the elements for that subject. I think that the logic should be to check if all items in the `element` array yielded at least one valid element.
synchon (Migrated from github.com) reviewed 2025-01-02 15:34:04 +00:00
@ -243,7 +263,7 @@ def queue(
kind: str,
jobname: str = "junifer_job",
synchon (Migrated from github.com) commented 2025-01-02 15:34:04 +00:00

Let me try to make it concrete and maybe it'll be easier to understand:

For this DataGrabber:

...
...
...
datagrabber:
  kind: DMCC13Benchmark
  types:
    - BOLD 
...
...
...

junifer list-elements <yaml> will give you:

...
...
...
sub-f9057kp,ses-wave1pro,Rest,AP,1
sub-f9057kp,ses-wave1pro,Rest,PA,2
sub-f9057kp,ses-wave1rea,Rest,AP,1
sub-f9057kp,ses-wave1rea,Rest,PA,2

Now if I do:

junifer run <yaml> --element sub-f9057kp

will run all "four" elements of the "subject".

Now, if I instead do:

junifer run <yaml> --element sub-f9057k

it'd just "exit" without the changes in this PR, but give you this:

...
...
...
RuntimeError: The following element selectors are invalid:
{'sub-f9057k'}

with the current PR. Also, doing this:

junifer run <yaml> --element sub-f9057kp,ses-wave1pr

would give you:

...
...
...
RuntimeError: The following element selectors are invalid:
{('sub-f9057kp', 'ses-wave1pr')}

Is this what you meant or something else?

Let me try to make it concrete and maybe it'll be easier to understand: For this DataGrabber: ```yaml ... ... ... datagrabber: kind: DMCC13Benchmark types: - BOLD ... ... ... ``` `junifer list-elements <yaml>` will give you: ```console ... ... ... sub-f9057kp,ses-wave1pro,Rest,AP,1 sub-f9057kp,ses-wave1pro,Rest,PA,2 sub-f9057kp,ses-wave1rea,Rest,AP,1 sub-f9057kp,ses-wave1rea,Rest,PA,2 ``` Now if I do: ```console junifer run <yaml> --element sub-f9057kp ``` will run all "four" elements of the "subject". Now, if I instead do: ```console junifer run <yaml> --element sub-f9057k ``` it'd just "exit" without the changes in this PR, but give you this: ```console ... ... ... RuntimeError: The following element selectors are invalid: {'sub-f9057k'} ``` with the current PR. Also, doing this: ```console junifer run <yaml> --element sub-f9057kp,ses-wave1pr ``` would give you: ```console ... ... ... RuntimeError: The following element selectors are invalid: {('sub-f9057kp', 'ses-wave1pr')} ``` Is this what you meant or something else?
fraimondo (Migrated from github.com) reviewed 2025-01-04 08:26:55 +00:00
@ -243,7 +263,7 @@ def queue(
kind: str,
jobname: str = "junifer_job",
fraimondo (Migrated from github.com) commented 2025-01-04 08:26:55 +00:00

That's what I meant. I'm kind of rusty now, but I had the impression that the PR would not account for that logic.

That's what I meant. I'm kind of rusty now, but I had the impression that the PR would not account for that logic.
synchon (Migrated from github.com) reviewed 2025-01-06 10:34:47 +00:00
@ -243,7 +263,7 @@ def queue(
kind: str,
jobname: str = "junifer_job",
synchon (Migrated from github.com) commented 2025-01-06 10:34:47 +00:00

Resolving it then.

Resolving it then.
fraimondo (Migrated from github.com) requested changes 2025-01-07 21:50:33 +00:00
fraimondo (Migrated from github.com) commented 2025-01-07 21:50:26 +00:00

Can you add a test case for the case we discussed?

Ej, sub-01 exists, but ses-02 does not.

Can you add a test case for the case we discussed? Ej, sub-01 exists, but ses-02 does not.
synchon (Migrated from github.com) reviewed 2025-01-08 15:06:37 +00:00
synchon (Migrated from github.com) commented 2025-01-08 15:06:37 +00:00

Added now.

Added now.
fraimondo (Migrated from github.com) approved these changes 2025-01-08 15:18:18 +00:00
fraimondo commented 2025-01-08 15:18:30 +00:00 (Migrated from github.com)

Once CI is green, LGTM!

Once CI is green, LGTM!
Sign in to join this conversation.
No reviewers
No milestone
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!319
No description provided.