[ENH] Upgrade custom entities #208

arnaudbore · 2023-06-09T17:25:35Z

No description provided.

codecov-commenter · 2023-06-13T02:40:07Z

Codecov Report

Merging #208 (817daa5) into master (d823293) will increase coverage by 1.26%.
The diff coverage is 87.34%.

@@            Coverage Diff             @@
##           master     #208      +/-   ##
==========================================
+ Coverage   78.78%   80.04%   +1.26%     
==========================================
  Files          15       15              
  Lines         773      832      +59     
  Branches      111      137      +26     
==========================================
+ Hits          609      666      +57     
  Misses        134      134              
- Partials       30       32       +2

Flag	Coverage Δ
pytest	`80.04% <87.34%> (+1.26%)`	⬆️

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

Impacted Files	Coverage Δ
dcm2bids/utils/io.py	`75.86% <ø> (+8.21%)`	⬆️
dcm2bids/sidecar.py	`86.69% <81.81%> (-0.81%)`	⬇️
dcm2bids/acquisition.py	`98.30% <100.00%> (+0.05%)`	⬆️
dcm2bids/cli/dcm2bids.py	`84.61% <100.00%> (+0.61%)`	⬆️
dcm2bids/dcm2bids_gen.py	`82.72% <100.00%> (+0.15%)`	⬆️
dcm2bids/utils/utils.py	`79.31% <100.00%> (+2.83%)`	⬆️

... and 2 files with indirect coverage changes

smeisler · 2023-06-21T20:49:22Z

Some feedback:

I like the new IntendedFor / id system
Auto extraction of direction worked on my fmaps, but it would be nice to also add dir-<> labels to BOLD and DWI, which did not happen automatically in my tests
When I auto-extracted features, the acq-<> label I manually specified seemed to have gotten ignored. I don’t know if this was intentional or not, but it would be nice if one can manually override auto-extraction (e.g., auto-extract direction, but manually add the acq label)
I was not able to test the auto extraction of task names because our files did not fit the regex naming convention, but it looks like a nice feature
Is there a way I can look at the a rendered documentation page for the development branch?

In relation to points 2-3, I have shared an example config and set of dicoms:

Desired tree:


└── ses-T1
    ├── anat
    │   ├── sub-READ1010_ses-T1_acq-VNavNorm_rec-defaced_T1w.json
    │   └── sub-READ1010_ses-T1_acq-VNavNorm_rec-defaced_T1w.nii.gz
    ├── dwi
    │   ├── sub-READ1010_ses-T1_acq-MDDW64_dir-PA_dwi.bval
    │   ├── sub-READ1010_ses-T1_acq-MDDW64_dir-PA_dwi.bvec
    │   ├── sub-READ1010_ses-T1_acq-MDDW64_dir-PA_dwi.json
    │   └── sub-READ1010_ses-T1_acq-MDDW64_dir-PA_dwi.nii.gz
    ├── fmap
    │   ├── sub-READ1010_ses-T1_acq-AM_dir-AP_epi.json
    │   ├── sub-READ1010_ses-T1_acq-AM_dir-AP_epi.nii.gz
    │   ├── sub-READ1010_ses-T1_acq-DWI_dir-AP_epi.json
    │   ├── sub-READ1010_ses-T1_acq-DWI_dir-AP_epi.nii.gz
    │   ├── sub-READ1010_ses-T1_acq-DWI_dir-PA_epi.json
    │   ├── sub-READ1010_ses-T1_acq-DWI_dir-PA_epi.nii.gz
    │   ├── sub-READ1010_ses-T1_acq-EF_dir-AP_epi.json
    │   ├── sub-READ1010_ses-T1_acq-EF_dir-AP_epi.nii.gz
    │   ├── sub-READ1010_ses-T1_acq-PSR_dir-AP_epi.json
    │   ├── sub-READ1010_ses-T1_acq-PSR_dir-AP_epi.nii.gz
    │   ├── sub-READ1010_ses-T1_acq-VM_dir-AP_epi.json
    │   └── sub-READ1010_ses-T1_acq-VM_dir-AP_epi.nii.gz
    └── func
        ├── sub-READ1010_ses-T1_task-AM_dir-PA_bold.json
        ├── sub-READ1010_ses-T1_task-AM_dir-PA_bold.nii.gz
        ├── sub-READ1010_ses-T1_task-EF_dir-PA_bold.json
        ├── sub-READ1010_ses-T1_task-EF_dir-PA_bold.nii.gz
        ├── sub-READ1010_ses-T1_task-PSR_dir-PA_bold.json
        ├── sub-READ1010_ses-T1_task-PSR_dir-PA_bold.nii.gz
        ├── sub-READ1010_ses-T1_task-VM_dir-PA_bold.json
        └── sub-READ1010_ses-T1_task-VM_dir-PA_bold.nii.gz

Actual tree with new config and automated label extraction. Note that the fmaps do not use the acq- label I supplied.

└── ses-T1
    ├── anat
    │   ├── sub-READ1010_ses-T1_acq-VNavNorm_T1w.json
    │   └── sub-READ1010_ses-T1_acq-VNavNorm_T1w.nii.gz
    ├── dwi
    │   ├── sub-READ1010_ses-T1_acq-MDDW64_dwi.bval
    │   ├── sub-READ1010_ses-T1_acq-MDDW64_dwi.bvec
    │   ├── sub-READ1010_ses-T1_acq-MDDW64_dwi.json
    │   └── sub-READ1010_ses-T1_acq-MDDW64_dwi.nii.gz
    ├── fmap
    │   ├── sub-READ1010_ses-T1_dir-PA_run-01_epi.json
    │   ├── sub-READ1010_ses-T1_dir-PA_run-01_epi.nii.gz
    │   ├── sub-READ1010_ses-T1_dir-PA_run-02_epi.json
    │   ├── sub-READ1010_ses-T1_dir-PA_run-02_epi.nii.gz
    │   ├── sub-READ1010_ses-T1_dir-PA_run-03_epi.json
    │   ├── sub-READ1010_ses-T1_dir-PA_run-03_epi.nii.gz
    │   ├── sub-READ1010_ses-T1_dir-PA_run-04_epi.json
    │   ├── sub-READ1010_ses-T1_dir-PA_run-04_epi.nii.gz
    │   ├── sub-READ1010_ses-T1_dir-PA_run-05_epi.json
    │   ├── sub-READ1010_ses-T1_dir-PA_run-05_epi.nii.gz
    │   ├── sub-READ1010_ses-T1_dir-PA_run-06_epi.json
    │   └── sub-READ1010_ses-T1_dir-PA_run-06_epi.nii.gz
    └── func
        ├── sub-READ1010_ses-T1_task-AM_bold.json
        ├── sub-READ1010_ses-T1_task-AM_bold.nii.gz
        ├── sub-READ1010_ses-T1_task-EF_bold.json
        ├── sub-READ1010_ses-T1_task-EF_bold.nii.gz
        ├── sub-READ1010_ses-T1_task-PSR_bold.json
        ├── sub-READ1010_ses-T1_task-PSR_bold.nii.gz
        ├── sub-READ1010_ses-T1_task-VM_bold.json
        └── sub-READ1010_ses-T1_task-VM_bold.nii.gz

arnaudbore · 2023-06-22T01:30:16Z

Some feedback:

I like the new IntendedFor / id system

Great feature indeed. Glad you like it! It makes everything easier especially for our users.

Auto extraction of direction worked on my fmaps, but it would be nice to also add dir-<> labels to BOLD and DWI, which did not happen automatically in my tests

Check the documentation I give more information about this auto_extract_entities new feature. If you check the BIDS specification dir is only mandatory for fmaps.

When I auto-extracted features, the acq-<> label I manually specified seemed to have gotten ignored. I don’t know if this was intentional or not, but it would be nice if one can manually override auto-extraction (e.g., auto-extract direction, but manually add the acq label)

Now it works the way it is suppose to work. :)

I was not able to test the auto extraction of task names because our files did not fit the regex naming convention, but it looks like a nice feature

It seems to work well on my side and I hope it will start some talks with MRI technologists.

Is there a way I can look at the a rendered documentation page for the development branch?

You can compile the documentation locally using this command line: portray as_html

smeisler · 2023-06-22T14:06:00Z

Check the documentation I give more information about this auto_extract_entities new feature. If you check the BIDS specification dir is only mandatory for fmaps.

Still, especially for those who collect reverse phase-encoded fmaps, having this information in the filenames (which is not-required but is still BIDS-valid for DWI/BOLD) could be helpful.

Now it works the way it is suppose to work. :)

Re-pulled the branch and retested - Indeed it does work!

I don't think I will have time to review the documentation before you plan to merge, but I would be happy to give feedback on that later. Great work!

arnaudbore · 2023-06-22T14:15:14Z

Check the documentation I give more information about this auto_extract_entities new feature. If you check the BIDS specification dir is only mandatory for fmaps.

Still, especially for those who collect reverse phase-encoded fmaps, having this information in the filenames (which is not-required but is still BIDS-valid for DWI/BOLD) could be helpful.

I agree and people can easily make it work for dwi and bold if needed. I'll see that in the documentation.

Now it works the way it is suppose to work. :)

Re-pulled the branch and retested - Indeed it does work!

I don't think I will have time to review the documentation before you plan to merge, but I would be happy to give feedback on that later. Great work!

Thank you! 🎉

arnaudbore added 6 commits June 8, 2023 22:00

change customLabels to customEntities

f161425

Major enh but still some bugs

3caf4c4

Some fixes

ac9caba

Merge branch 'master' into enh_custom_labels

fffb1c5

remove print

aaf9c46

some bugs fixed

5ed3b78

arnaudbore changed the title ~~[WIP][ENH] Upgrade custom entities~~ [ENH] Upgrade custom entities Jun 13, 2023

arnaudbore added 7 commits June 12, 2023 23:07

add options for dir entity

a28c8a4

big chunk of bugs fixed

2b4f97f

add test file

98b02f2

improve test

5f7b8a7

fix tests env + some cleaning

340d3bf

bug fix - #1

2aa572f

first modification of the doc

d1f29b8

arnaudbore mentioned this pull request Jun 20, 2023

[ENH][WIP] Add a new customLabel feature using dcmTags for all acquisitions #60

Closed

arnaudbore added 3 commits June 21, 2023 10:22

fix typo sam

9abd3ce

fix searchDcmTagEntity when looking for int or float

3ffb5e0

improve doc

a64fadf

arnaudbore added 3 commits June 21, 2023 21:01

fix customEntities and dir auto-entity

172d5f5

add auto extract entities log

8560a50

fix test

650ad09

arnaudbore added 2 commits June 22, 2023 13:41

fix some logs

817daa5

reorder when runs

ab33266

arnaudbore merged commit 86f89e3 into UNFmontreal:master Jun 22, 2023

arnaudbore deleted the enh_custom_labels branch July 3, 2023 22:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ENH] Upgrade custom entities #208

[ENH] Upgrade custom entities #208

arnaudbore commented Jun 9, 2023

codecov-commenter commented Jun 13, 2023 •

edited

Loading

smeisler commented Jun 21, 2023 •

edited

Loading

arnaudbore commented Jun 22, 2023

smeisler commented Jun 22, 2023

arnaudbore commented Jun 22, 2023

[ENH] Upgrade custom entities #208

[ENH] Upgrade custom entities #208

Conversation

arnaudbore commented Jun 9, 2023

codecov-commenter commented Jun 13, 2023 • edited Loading

Codecov Report

smeisler commented Jun 21, 2023 • edited Loading

arnaudbore commented Jun 22, 2023

smeisler commented Jun 22, 2023

arnaudbore commented Jun 22, 2023

codecov-commenter commented Jun 13, 2023 •

edited

Loading

smeisler commented Jun 21, 2023 •

edited

Loading