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

[BUG]: subject ID truncated incorrectly in condor submit dag file #322

Merged
synchon merged 2 commits from fix/subject-id-truncation into main 2024-04-05 14:22:19 +00:00
Conversation 22 Commits 2 Files changed 3 +19 -4
synchon commented 2024-04-05 11:55:53 +00:00 (Migrated from github.com)
Copy link

Is there an existing issue for this?

  • I have searched the existing issues

Current Behavior

First I queued using the datagrabber without specifying the elements. The resulting file gives elements such as "0001" etc.

When I supply a txt file with a subset of elements I want to process, the subject ID's have the 0 buffers removed.

Expected Behavior

It should output the ID's as a string so the buffering 0's should still be there.

Steps To Reproduce

  1. See the environment posted below.
  2. See the YAML i used:
workdir: /tmp

datagrabber:
    kind: DataladAOMICID1000

preprocess:
    kind: Smoothing
    using: "nilearn"
    "on": "BOLD"
    smoothing_params:
      fwhm: 10

storage:
  kind: HDF5FeatureStorage
  uri: data/aomic_id1000.hdf5


markers:

  - name: parccortical-Schaefer100x17FSLMNI_marker-empiricalFC
    kind: FunctionalConnectivityParcels
    parcellation: Schaefer100x17
    cor_method: correlation
    cor_method_params:
      empirical: true
    masks:
      - inherit
        
queue:
  jobname: smoothing_test
  kind: HTCondor
  collect: on_success_only
  env:
    kind: venv
    name: env
  mem: 20G
  verbose: 8
  1. I create a txt file in the working directory (which is also where I have the yaml) as elements.txt:
"0001"
"0002"
"0100"
"0150"

I then run junifer queue test.yaml --element elements.txt

I tried this with and without quotes but both result in the wrong output (snippet from the result dag file):

JOB run0 /data/project/identification_prediction/projects/test_junifer_smoothing/junifer_jobs/smoothing_test/run_smoothing_test.submit
VARS run0 element="1" log_element="1"

JOB run1 /data/project/identification_prediction/projects/test_junifer_smoothing/junifer_jobs/smoothing_test/run_smoothing_test.submit
VARS run1 element="2" log_element="2"

JOB run2 /data/project/identification_prediction/projects/test_junifer_smoothing/junifer_jobs/smoothing_test/run_smoothing_test.submit
VARS run2 element="100" log_element="100"

JOB run3 /data/project/identification_prediction/projects/test_junifer_smoothing/junifer_jobs/smoothing_test/run_smoothing_test.submit
VARS run3 element="150" log_element="150"


JOB collect /data/project/identification_prediction/projects/test_junifer_smoothing/junifer_jobs/smoothing_test/collect_smoothing_test.submit
PARENT run0 run1 run2 run3 CHILD collect

I then run the junifer queue aomic_id1000.yaml to see what the file should look like, here the result looks as follows (only a snippet):

JOB run813 /data/project/identification_prediction/projects/test_junifer_smoothing/junifer_jobs/smoothing_test/run_smoothing_test.submit
VARS run813 element="0329" log_element="0329"

JOB run814 /data/project/identification_prediction/projects/test_junifer_smoothing/junifer_jobs/smoothing_test/run_smoothing_test.submit
VARS run814 element="0603" log_element="0603"

JOB run815 /data/project/identification_prediction/projects/test_junifer_smoothing/junifer_jobs/smoothing_test/run_smoothing_test.submit
VARS run815 element="0463" log_element="0463"

JOB run816 /data/project/identification_prediction/projects/test_junifer_smoothing/junifer_jobs/smoothing_test/run_smoothing_test.submit
VARS run816 element="0911" log_element="0911"

JOB run817 /data/project/identification_prediction/projects/test_junifer_smoothing/junifer_jobs/smoothing_test/run_smoothing_test.submit
VARS run817 element="0791" log_element="0791"

Environment

❱ junifer wtf
junifer:
  version: 0.0.4.dev822
python:
  version: 3.10.10
  implementation: CPython
dependencies:
  click: 8.1.7
  numpy: 1.26.4
  scipy: 1.11.4
  datalad: 0.19.6
  pandas: 2.1.4
  nibabel: 5.2.1
  nilearn: 0.10.2
  sqlalchemy: 2.0.28
  ruamel.yaml: 0.17.21
  httpx: 0.26.0
  tqdm: 4.66.1
  templateflow: 24.2.0
  looseversion: None
system:
  platform: Linux-6.6.13+bpo-amd64-x86_64-with-glibc2.36
environment:
  LC_CTYPE: en_US.UTF-8
  PATH: /home/lsasse/miniconda3/bin:/home/lsasse/miniconda3/condabin:/home/lsasse/bin:/home/lsasse/.dotfiles/bin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:/usr/X11R6/bin:/usr/local/games:/usr/games

Relevant log output

No response

Anything else?

No response

### Is there an existing issue for this? - [X] I have searched the existing issues ### Current Behavior First I queued using the datagrabber without specifying the elements. The resulting file gives elements such as "0001" etc. When I supply a txt file with a subset of elements I want to process, the subject ID's have the 0 buffers removed. ### Expected Behavior It should output the ID's as a string so the buffering 0's should still be there. ### Steps To Reproduce 1. See the environment posted below. 2. See the YAML i used: ``` workdir: /tmp datagrabber: kind: DataladAOMICID1000 preprocess: kind: Smoothing using: "nilearn" "on": "BOLD" smoothing_params: fwhm: 10 storage: kind: HDF5FeatureStorage uri: data/aomic_id1000.hdf5 markers: - name: parccortical-Schaefer100x17FSLMNI_marker-empiricalFC kind: FunctionalConnectivityParcels parcellation: Schaefer100x17 cor_method: correlation cor_method_params: empirical: true masks: - inherit queue: jobname: smoothing_test kind: HTCondor collect: on_success_only env: kind: venv name: env mem: 20G verbose: 8 ``` 3. I create a txt file in the working directory (which is also where I have the yaml) as elements.txt: ``` "0001" "0002" "0100" "0150" ``` I then run `junifer queue test.yaml --element elements.txt` I tried this with and without quotes but both result in the wrong output (snippet from the result dag file): ``` JOB run0 /data/project/identification_prediction/projects/test_junifer_smoothing/junifer_jobs/smoothing_test/run_smoothing_test.submit VARS run0 element="1" log_element="1" JOB run1 /data/project/identification_prediction/projects/test_junifer_smoothing/junifer_jobs/smoothing_test/run_smoothing_test.submit VARS run1 element="2" log_element="2" JOB run2 /data/project/identification_prediction/projects/test_junifer_smoothing/junifer_jobs/smoothing_test/run_smoothing_test.submit VARS run2 element="100" log_element="100" JOB run3 /data/project/identification_prediction/projects/test_junifer_smoothing/junifer_jobs/smoothing_test/run_smoothing_test.submit VARS run3 element="150" log_element="150" JOB collect /data/project/identification_prediction/projects/test_junifer_smoothing/junifer_jobs/smoothing_test/collect_smoothing_test.submit PARENT run0 run1 run2 run3 CHILD collect ``` I then run the `junifer queue aomic_id1000.yaml` to see what the file **should** look like, here the result looks as follows (only a snippet): ``` JOB run813 /data/project/identification_prediction/projects/test_junifer_smoothing/junifer_jobs/smoothing_test/run_smoothing_test.submit VARS run813 element="0329" log_element="0329" JOB run814 /data/project/identification_prediction/projects/test_junifer_smoothing/junifer_jobs/smoothing_test/run_smoothing_test.submit VARS run814 element="0603" log_element="0603" JOB run815 /data/project/identification_prediction/projects/test_junifer_smoothing/junifer_jobs/smoothing_test/run_smoothing_test.submit VARS run815 element="0463" log_element="0463" JOB run816 /data/project/identification_prediction/projects/test_junifer_smoothing/junifer_jobs/smoothing_test/run_smoothing_test.submit VARS run816 element="0911" log_element="0911" JOB run817 /data/project/identification_prediction/projects/test_junifer_smoothing/junifer_jobs/smoothing_test/run_smoothing_test.submit VARS run817 element="0791" log_element="0791" ``` ### Environment ```markdown ❱ junifer wtf junifer: version: 0.0.4.dev822 python: version: 3.10.10 implementation: CPython dependencies: click: 8.1.7 numpy: 1.26.4 scipy: 1.11.4 datalad: 0.19.6 pandas: 2.1.4 nibabel: 5.2.1 nilearn: 0.10.2 sqlalchemy: 2.0.28 ruamel.yaml: 0.17.21 httpx: 0.26.0 tqdm: 4.66.1 templateflow: 24.2.0 looseversion: None system: platform: Linux-6.6.13+bpo-amd64-x86_64-with-glibc2.36 environment: LC_CTYPE: en_US.UTF-8 PATH: /home/lsasse/miniconda3/bin:/home/lsasse/miniconda3/condabin:/home/lsasse/bin:/home/lsasse/.dotfiles/bin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:/usr/X11R6/bin:/usr/local/games:/usr/games ``` ### Relevant log output _No response_ ### Anything else? _No response_
fraimondo commented 2024-04-05 10:42:41 +00:00 (Migrated from github.com)
Copy link

We are using only strings for replacements for AOMIC instead of "sub-0001". I was having that problem yesterday, as the numbers were converted to ints.

I know that maybe this is not the place, but we should try to keep with the BIDS standard and use the subjects with the prefix.

I want to merge the junifer DF with the participants.tsv and I need to manipulate the index.

We are using only strings for replacements for AOMIC instead of "sub-0001". I was having that problem yesterday, as the numbers were converted to ints. I know that maybe this is not the place, but we should try to keep with the BIDS standard and use the subjects with the _prefix_. I want to merge the junifer DF with the participants.tsv and I need to manipulate the index.
fraimondo commented 2024-04-05 10:44:00 +00:00 (Migrated from github.com)
Copy link

So for this bug, the parse elements should not convert from text to int. But how can we know when it's ok to do it? or when it's not?

So for this bug, the parse elements should not convert from text to int. But how can we know when it's ok to do it? or when it's not?
👍 1
LeSasse commented 2024-04-05 10:45:14 +00:00 (Migrated from github.com)
Copy link

the elements should always be string literals in my opinion. Maybe that requires changing some data grabber implementations? but that is in my opinion the easiest and best solution

the elements should always be string literals in my opinion. Maybe that requires changing some data grabber implementations? but that is in my opinion the easiest and best solution
fraimondo commented 2024-04-05 10:47:58 +00:00 (Migrated from github.com)
Copy link

the elements should always be string literals in my opinion. Maybe that requires changing some data grabber implementations? but that is in my opinion the easiest and best solution

Not at all. I think it's the element parser that should not convert to int. Indeed you can see here that some of this things is converting to int. Even though we specifically want strings.

github.com/juaml/junifer@a9e799caa3/junifer/api/cli.py (L98-L124)

> the elements should always be string literals in my opinion. Maybe that requires changing some data grabber implementations? but that is in my opinion the easiest and best solution Not at all. I think it's the element parser that should not convert to int. Indeed you can see here that some of this things is converting to int. Even though we specifically want strings. https://github.com/juaml/junifer/blob/a9e799caa3433d3d4eaa633d4565771b8eb297f6/junifer/api/cli.py#L98-L124
LeSasse commented 2024-04-05 10:51:06 +00:00 (Migrated from github.com)
Copy link

converting to int

Yeah it should never do that in my opinion. I would always expect that the elements in the text files are seen as strings.

> converting to int Yeah it should never do that in my opinion. I would always expect that the elements in the text files are seen as strings.
synchon commented 2024-04-05 10:52:37 +00:00 (Migrated from github.com)
Copy link

Following the BIDS standard and having sub- prefix in my opinion is also the best way. The way pandas.DataFrames handles data loading from CSV is kind of the issue here. Although, the code is meant to convert as much as possible to str, it misses this and we get this issue.

Following the BIDS standard and having `sub-` prefix in my opinion is also the best way. The way pandas.DataFrames handles data loading from CSV is kind of the issue here. Although, the code is meant to convert as much as possible to str, it misses this and we get this issue.
👍 1
LeSasse commented 2024-04-05 10:53:09 +00:00 (Migrated from github.com)
Copy link

maybe https://numpy.org/doc/1.18/reference/generated/numpy.loadtxt.html is better suited in this case since you can specify a delimiter and tell it the dtype=str so then it should all be loaded as str

maybe https://numpy.org/doc/1.18/reference/generated/numpy.loadtxt.html is better suited in this case since you can specify a delimiter and tell it the dtype=str so then it should all be loaded as str
synchon commented 2024-04-05 10:55:27 +00:00 (Migrated from github.com)
Copy link

Here's a relevant issue:
https://stackoverflow.com/questions/16988526/pandas-reading-csv-as-string-type

The pandas type sniffer is the culprit from what I understand.

Here's a relevant issue: https://stackoverflow.com/questions/16988526/pandas-reading-csv-as-string-type The pandas type sniffer is the culprit from what I understand.
LeSasse commented 2024-04-05 10:55:31 +00:00 (Migrated from github.com)
Copy link

Also I agree with the BIDS standard thing, but still think the bug is relevant since not all datasets follow the BIDS standard (e.g. HCP also uses integers only as subject index, although the bug likely wont happen there for different reasons). So i think getting as much "type safety" as possible here will be good nonetheless

Also I agree with the BIDS standard thing, but still think the bug is relevant since not all datasets follow the BIDS standard (e.g. HCP also uses integers only as subject index, although the bug likely wont happen there for different reasons). So i think getting as much "type safety" as possible here will be good nonetheless
LeSasse commented 2024-04-05 10:57:16 +00:00 (Migrated from github.com)
Copy link

Here's a relevant issue: https://stackoverflow.com/questions/16988526/pandas-reading-csv-as-string-type

The pandas type sniffer is the culprit from what I understand.

yes, i think np.loadtxt will be better here as far as i understand

> Here's a relevant issue: https://stackoverflow.com/questions/16988526/pandas-reading-csv-as-string-type > > The pandas type sniffer is the culprit from what I understand. yes, i think np.loadtxt will be better here as far as i understand
fraimondo commented 2024-04-05 10:58:54 +00:00 (Migrated from github.com)
Copy link

We need to also be able to read elements as tuples, so basically, CSV files too

We need to also be able to read elements as tuples, so basically, CSV files too
LeSasse commented 2024-04-05 11:02:13 +00:00 (Migrated from github.com)
Copy link

We need to also be able to read elements as tuples, so basically, CSV files too

np.loadtxt can do that, it has a delimiter parameter and reads in columns to have 2D arrays of a particular type as far as i know

> We need to also be able to read elements as tuples, so basically, CSV files too np.loadtxt can do that, it has a delimiter parameter and reads in columns to have 2D arrays of a particular type as far as i know
LeSasse commented 2024-04-05 11:02:31 +00:00 (Migrated from github.com)
Copy link

might be worth testing out

might be worth testing out
synchon commented 2024-04-05 11:10:00 +00:00 (Migrated from github.com)
Copy link

Okay I tested np.loadtxt and it works correctly, will make a quick PR out of this.

Okay I tested `np.loadtxt` and it works correctly, will make a quick PR out of this.
synchon commented 2024-04-05 11:50:15 +00:00 (Migrated from github.com)
Copy link

Okay my bad, I overlooked the pandas version which we support, so I got it working with minimal change and using pandas. I would not want to change a lot just to not have unexpected changes before a release.

Okay my bad, I overlooked the pandas version which we support, so I got it working with minimal change and using pandas. I would not want to change a lot just to not have unexpected changes before a release.
👍 1
LeSasse commented 2024-04-05 11:51:35 +00:00 (Migrated from github.com)
Copy link

makes sense, if there is a way to make it work with pandas, then thats preferred i think

makes sense, if there is a way to make it work with pandas, then thats preferred i think
LeSasse (Migrated from github.com) reviewed 2024-04-05 11:55:53 +00:00
synchon commented 2024-04-05 11:57:48 +00:00 (Migrated from github.com)
Copy link

@LeSasse @fraimondo can you run your YAMLs with this branch and see if it works as expected?

@LeSasse @fraimondo can you run your YAMLs with this branch and see if it works as expected?
github-actions[bot] commented 2024-04-05 12:19:53 +00:00 (Migrated from github.com)
Copy link
PR Preview Action v1.4.7
Preview removed because the pull request was closed.
2024-04-05 14:44 UTC
[PR Preview Action](https://github.com/rossjrw/pr-preview-action) v1.4.7 :---: Preview removed because the pull request was closed. 2024-04-05 14:44 UTC <!-- Sticky Pull Request Commentpr-preview -->
fraimondo (Migrated from github.com) approved these changes 2024-04-05 12:25:35 +00:00
LeSasse commented 2024-04-05 12:33:00 +00:00 (Migrated from github.com)
Copy link

@LeSasse @fraimondo can you run your YAMLs with this branch and see if it works as expected?

works for me now

> @LeSasse @fraimondo can you run your YAMLs with this branch and see if it works as expected? works for me now
codecov[bot] commented 2024-04-05 13:08:09 +00:00 (Migrated from github.com)
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (3a36ad0) to head (c284d76).

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff            @@
##              main      #322   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            1         1           
  Lines            1         1           
=========================================
  Hits             1         1
Flag Coverage Δ
docs 100.00% <ø> (ø)

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

## [Codecov](https://app.codecov.io/gh/juaml/junifer/pull/322?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml) Report All modified and coverable lines are covered by tests :white_check_mark: > Project coverage is 100.00%. Comparing base [(`3a36ad0`)](https://app.codecov.io/gh/juaml/junifer/commit/3a36ad0ac258c695793e09ad055ce0bac5ce195e?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml) to head [(`c284d76`)](https://app.codecov.io/gh/juaml/junifer/pull/322?dropdown=coverage&src=pr&el=desc&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/322/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/322?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml) ```diff @@ Coverage Diff @@ ## main #322 +/- ## ========================================= Coverage 100.00% 100.00% ========================================= Files 1 1 Lines 1 1 ========================================= Hits 1 1 ``` | [Flag](https://app.codecov.io/gh/juaml/junifer/pull/322/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/322/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml) | `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. </details>
Sign in to join this conversation.
Reviewers
No reviewers
LeSasse
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!322
No description provided.