➕ Reorient resources to set orientation in config #2140

birajstha · 2024-08-16T21:57:52Z

Related to

Description

This PR fixes the different orientation in CPAC output dir that were caused by the different orientation of templates being ingressed.

Added reorientation to the templates that are being ingressed and /or resampled (for different resolution) in CPAC.
Added desired_orientation as config key in blank-config and set it default to RPI
With this PR the desired orientation of the outputs can be selected by modifying this key in the config.

Technical details

First of all,
The reorientation of templates are done as below

resolve_resolution function is modified to take in orientation as extra input.
Orientation inputs are taken from the config and are added in the following spots

Tests

For testing you can run this branch of CPAC on one test subject / session with your config (set as desired_orientation) and check the orientations of the images in the output directory both anat and func

cd into the anat or func directory.
run for file in *.nii.gz; do orientation=$(3dinfo -orient "$file" 2>/dev/null); echo "$file: $orientation"; done

You should see the filename with orientations as below
anat :

func :

Screenshots

Problem: Different orientations in the CPAC output dir

anat dir:
func dir:

After fix:

anat dir:
func dir:

Checklist

My pull request has a descriptive title (not a vague title like Update index.md).
My pull request targets the develop branch of the repository.
My commit messages follow best practices.
My code follows the established code style of the repository.
I added tests for the changes I made (if applicable).
I updated the changelog.
I added or updated documentation (if applicable).
I tried running the project locally and verified that there are no visible errors.

Developer Certificate of Origin

Developer Certificate of Origin
Version 1.1

Copyright (C) 2004, 2006 The Linux Foundation and its contributors.
1 Letterman Drive
Suite D4700
San Francisco, CA, 94129

Everyone is permitted to copy and distribute verbatim copies of this
license document, but changing it is not allowed.


Developer's Certificate of Origin 1.1

By making a contribution to this project, I certify that:

(a) The contribution was created in whole or in part by me and I
    have the right to submit it under the open source license
    indicated in the file; or

(b) The contribution is based upon previous work that, to the best
    of my knowledge, is covered under an appropriate open source
    license and I have the right under that license to submit that
    work with modifications, whether created in whole or in part
    by me, under the same open source license (unless I am
    permitted to submit under a different license), as indicated
    in the file; or

(c) The contribution was provided directly to me by some other
    person who certified (a), (b) or (c) and I have not modified
    it.

(d) I understand and agree that this project and the contribution
    are public and that a record of the contribution (including all
    personal information I submit with it, including my sign-off) is
    maintained indefinitely and may be redistributed consistent with
    this project or the open source license(s) involved.

shnizzedy

This comment is my only substantive question & suggestion.

The rest is style that the ruff-format hook in pre-commit should have caught.

(I cherry-pick'd these changes locally and ran pre-commit run on them to catch these after I noticed a couple. Of

the errors that ruff found

CPAC/pipeline/engine.py:1:1: D100 Missing docstring in public module
CPAC/pipeline/engine.py:73:7: D101 Missing docstring in public class
CPAC/pipeline/engine.py:136:9: D105 Missing docstring in magic method
CPAC/pipeline/engine.py:144:9: D105 Missing docstring in magic method
CPAC/pipeline/engine.py:149:9: D102 Missing docstring in public method
CPAC/pipeline/engine.py:197:9: D102 Missing docstring in public method
CPAC/pipeline/engine.py:200:9: D102 Missing docstring in public method
CPAC/pipeline/engine.py:208:9: D102 Missing docstring in public method
CPAC/pipeline/engine.py:211:9: D102 Missing docstring in public method
CPAC/pipeline/engine.py:214:9: D102 Missing docstring in public method
CPAC/pipeline/engine.py:217:9: D102 Missing docstring in public method
CPAC/pipeline/engine.py:220:9: D102 Missing docstring in public method
CPAC/pipeline/engine.py:223:9: D102 Missing docstring in public method
CPAC/pipeline/engine.py:240:9: D102 Missing docstring in public method
CPAC/pipeline/engine.py:257:9: D102 Missing docstring in public method
CPAC/pipeline/engine.py:270:9: D102 Missing docstring in public method
CPAC/pipeline/engine.py:278:9: D102 Missing docstring in public method
CPAC/pipeline/engine.py:328:9: D102 Missing docstring in public method
CPAC/pipeline/engine.py:396:9: D102 Missing docstring in public method
CPAC/pipeline/engine.py:437:9: D102 Missing docstring in public method
CPAC/pipeline/engine.py:455:9: D102 Missing docstring in public method
CPAC/pipeline/engine.py:462:9: D102 Missing docstring in public method
CPAC/pipeline/engine.py:466:9: D102 Missing docstring in public method
CPAC/pipeline/engine.py:469:9: D102 Missing docstring in public method
CPAC/pipeline/engine.py:491:9: D102 Missing docstring in public method
CPAC/pipeline/engine.py:504:9: D102 Missing docstring in public method
CPAC/pipeline/engine.py:519:9: D102 Missing docstring in public method
CPAC/pipeline/engine.py:529:9: D102 Missing docstring in public method
CPAC/pipeline/engine.py:549:9: D102 Missing docstring in public method
CPAC/pipeline/engine.py:562:9: PLR0912 Too many branches (69 > 50)
CPAC/pipeline/engine.py:562:9: PLR0915 Too many statements (155 > 100)
CPAC/pipeline/engine.py:562:9: D102 Missing docstring in public method
CPAC/pipeline/engine.py:814:9: D102 Missing docstring in public method
CPAC/pipeline/engine.py:930:9: D102 Missing docstring in public method
CPAC/pipeline/engine.py:1073:9: PLR0912 Too many branches (69 > 50)
CPAC/pipeline/engine.py:1073:9: PLR0915 Too many statements (171 > 100)
CPAC/pipeline/engine.py:1073:9: D102 Missing docstring in public method
CPAC/pipeline/engine.py:1073:37: A002 Argument `all` is shadowing a Python builtin
CPAC/pipeline/engine.py:1420:7: D101 Missing docstring in public class
CPAC/pipeline/engine.py:1431:17: PLW2901 `for` loop variable `node_block_function` overwritten by assignment target
CPAC/pipeline/engine.py:1492:9: D102 Missing docstring in public method
CPAC/pipeline/engine.py:1495:9: D102 Missing docstring in public method
CPAC/pipeline/engine.py:1500:9: D102 Missing docstring in public method
CPAC/pipeline/engine.py:1509:9: D102 Missing docstring in public method
CPAC/pipeline/engine.py:1519:9: PLR0912 Too many branches (91 > 50)
CPAC/pipeline/engine.py:1519:9: PLR0915 Too many statements (186 > 100)
CPAC/pipeline/engine.py:1519:9: D102 Missing docstring in public method
CPAC/pipeline/engine.py:1831:37: PLW2901 `for` loop variable `connection` overwritten by assignment target
CPAC/pipeline/engine.py:1910:5: D103 Missing docstring in public function
CPAC/pipeline/engine.py:1955:5: D103 Missing docstring in public function
CPAC/pipeline/engine.py:2047:5: D103 Missing docstring in public function
CPAC/pipeline/engine.py:2093:5: D103 Missing docstring in public function
CPAC/pipeline/engine.py:2216:17: E722 Do not use bare `except`
CPAC/pipeline/engine.py:2240:5: D103 Missing docstring in public function
CPAC/pipeline/engine.py:2315:5: D103 Missing docstring in public function
CPAC/pipeline/engine.py:2383:5: D103 Missing docstring in public function
CPAC/pipeline/engine.py:2391:5: D103 Missing docstring in public function
CPAC/pipeline/engine.py:2411:5: D103 Missing docstring in public function
CPAC/pipeline/engine.py:2680:9: E722 Do not use bare `except`
CPAC/pipeline/engine.py:2697:5: D103 Missing docstring in public function
CPAC/pipeline/utils.py:29:5: D103 Missing docstring in public function
CPAC/pipeline/utils.py:39:12: RET504 Unnecessary assignment to `orientation` before `return` statement
CPAC/pipeline/utils.py:42:5: D103 Missing docstring in public function
Found 63 errors.

I'd add docstrings for

CPAC/pipeline/utils.py:29:5: D103 Missing docstring in public function
CPAC/pipeline/utils.py:42:5: D103 Missing docstring in public function

and change

C-PAC/CPAC/pipeline/utils.py

Lines 40 to 41 in dbdacd1

    
           orientation = subprocess.run(cmd_3dinfo, capture_output=True, text=True).stdout.strip().upper() 
        
           return orientation

to

    return subprocess.run(cmd_3dinfo, capture_output=True, text=True).stdout.strip().upper()

(or

    orientation = subprocess.run(cmd_3dinfo, capture_output=True, text=True).stdout.strip().upper() # noqa: RET504
    return orientation

to mark the style violation as intentional if you think it's that much more readable) for

CPAC/pipeline/utils.py:39:12: RET504 Unnecessary assignment to `orientation` before `return` statement

. Once all the remaining ruff violations are outside of the scope of the changes of this PR, I'd git commit --no-verify to accept those style violations as acceptable for now.)

CPAC/pipeline/engine.py

CPAC/pipeline/utils.py

shnizzedy

To fix:

broken calls via API changes:
- resolve_resolution
- ingress_pipeconfig_paths
~~T1w~~ → T2w (would be unnecessary if we do this optional change)

The SSOTing by functionalizing the creation of resample nodes is totally optional, just a suggestion.

CPAC/pipeline/engine.py

CPAC/utils/datasource.py

CPAC/pipeline/engine.py

CPAC/utils/datasource.py

Co-authored-by: Jon Clucas <[email protected]>

for more information, see https://pre-commit.ci

…nodes

for more information, see https://pre-commit.ci

…n is being used

…tation set in config in the existing nodes for reorientations

for more information, see https://pre-commit.ci

shnizzedy

This looks good, and all the tests are passing (including the smoke tests I manually triggered to check that all the preconfigs build with these changes)!

I have a few suggestions and a question for the CHANGELOG, and one more request to fix a style violation, but otherwise I'm happy with this fix. Nice work!

CHANGELOG.md

CPAC/resources/tests/test_templates.py

CHANGELOG.md

Co-authored-by: Jon Clucas <[email protected]>

Co-authored-by: Jon Cluce <[email protected]>

shnizzedy

🎉

birajstha had a problem deploying to ACCESS August 16, 2024 21:57 — with GitHub Actions Failure

birajstha linked an issue Aug 16, 2024 that may be closed by this pull request

✨ Nifti header orientation checks and decisions for downstream resources #2138

Open

1 task

shnizzedy requested changes Aug 23, 2024

View reviewed changes

birajstha removed a link to an issue Sep 20, 2024

✨ Nifti header orientation checks and decisions for downstream resources #2138

Open

1 task

birajstha linked an issue Sep 20, 2024 that may be closed by this pull request

✨ Nifti header orientation checks and decisions for downstream resources #2138

Open

1 task

birajstha changed the title ~~➕ orientation checks for resources added to PipeConfigs~~ ➕ orientation checks for resources Sep 26, 2024

birajstha changed the title ~~➕ orientation checks for resources~~ ➕ Reorient resources to set orientation in config Sep 27, 2024

shnizzedy requested changes Sep 27, 2024

View reviewed changes

shnizzedy reviewed Sep 30, 2024

View reviewed changes

CPAC/utils/datasource.py Outdated Show resolved Hide resolved

birajstha force-pushed the feature/check_orientations branch 3 times, most recently from c9e71f2 to 40ea22c Compare October 1, 2024 18:42

birajstha requested a review from shnizzedy October 2, 2024 18:22

birajstha self-assigned this Oct 2, 2024

Biraj Shrestha and others added 16 commits October 2, 2024 19:39

➕ orientation checks for resources added to PipeConfigs

b9867c9

Update CPAC/pipeline/utils.py

81869e2

Co-authored-by: Jon Clucas <[email protected]>

Update CPAC/pipeline/engine.py

0a32a32

Co-authored-by: Jon Clucas <[email protected]>

Update CPAC/pipeline/engine.py

d005b01

Co-authored-by: Jon Clucas <[email protected]>

Update CPAC/pipeline/engine.py

d31b1d9

Co-authored-by: Jon Clucas <[email protected]>

Update CPAC/pipeline/engine.py

5480678

Co-authored-by: Jon Clucas <[email protected]>

Update CPAC/pipeline/engine.py

b965952

Co-authored-by: Jon Clucas <[email protected]>

Update CPAC/pipeline/utils.py

9df6ab5

Co-authored-by: Jon Clucas <[email protected]>

Update CPAC/pipeline/utils.py

c0006cd

Co-authored-by: Jon Clucas <[email protected]>

Update CPAC/pipeline/utils.py

edf6d99

Co-authored-by: Jon Clucas <[email protected]>

Update CPAC/pipeline/utils.py

57d6133

Co-authored-by: Jon Clucas <[email protected]>

Update CPAC/pipeline/utils.py

0f190d8

Co-authored-by: Jon Clucas <[email protected]>

adding desired-orientation as a config key

c5b4a1e

[pre-commit.ci] auto fixes from pre-commit.com hooks

a04d47d

for more information, see https://pre-commit.ci

reorienting all files before rpool initialization

f2be81a

[pre-commit.ci] auto fixes from pre-commit.com hooks

995163c

for more information, see https://pre-commit.ci

birajstha and others added 17 commits October 2, 2024 19:39

moved orientation check and reorient if necessary to the wf

5aeebbb

[pre-commit.ci] auto fixes from pre-commit.com hooks

e8baa28

for more information, see https://pre-commit.ci

renaming output name correctly

ef10e36

handle for read-only template files

6bfd680

adding reorient nodes in anat, func, freesurfer and template ingress …

e454cec

…nodes

fix a typo in orientation flag for resample

d47defb

[pre-commit.ci] auto fixes from pre-commit.com hooks

c9b69b9

for more information, see https://pre-commit.ci

updating the orientation flag at other places where resolve_resolutio…

d0a7c2a

…n is being used

reverting adding reorient node at the very begining and passing orien…

7246e44

…tation set in config in the existing nodes for reorientations

[pre-commit.ci] auto fixes from pre-commit.com hooks

f3f3f98

for more information, see https://pre-commit.ci

empty

1a43750

updated tests

939c7a5

updated the changelog

9df743c

replaced RPI to take in config value in few more places

079b1f0

adding cfg as parameter in lesion_preproc

35ed398

sending only orientation not all cfg

b368083

fixed positions of args in lesion_preproc and added default RPI

71dd070

birajstha force-pushed the feature/check_orientations branch from 96f5871 to 71dd070 Compare October 3, 2024 00:39

shnizzedy requested changes Oct 9, 2024

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

CHANGELOG.md Outdated Show resolved Hide resolved

CPAC/resources/tests/test_templates.py Outdated Show resolved Hide resolved

CHANGELOG.md Outdated Show resolved Hide resolved

birajstha and others added 4 commits October 9, 2024 13:34

Update CHANGELOG.md

3f1e109

Co-authored-by: Jon Clucas <[email protected]>

Update CHANGELOG.md

d24fae6

Co-authored-by: Jon Clucas <[email protected]>

Update CHANGELOG.md

408d871

Co-authored-by: Jon Cluce <[email protected]>

fixing pre-commit suggestions

2895e8c

birajstha force-pushed the feature/check_orientations branch from 1b05911 to 2895e8c Compare October 10, 2024 16:23

Merge branch 'develop' into feature/check_orientations

5eedfb7

birajstha requested a review from shnizzedy October 10, 2024 17:38

shnizzedy approved these changes Oct 14, 2024

View reviewed changes

Merge branch 'develop' into feature/check_orientations

312ef00

sgiavasis merged commit cda28cd into develop Oct 14, 2024
24 checks passed

shnizzedy mentioned this pull request Nov 5, 2024

➕ Reorient resources to set orientation in config #2161

Open

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

➕ Reorient resources to set orientation in config #2140

➕ Reorient resources to set orientation in config #2140

birajstha commented Aug 16, 2024 •

edited

Loading

shnizzedy left a comment •

edited

Loading

shnizzedy left a comment •

edited by birajstha

Loading

shnizzedy left a comment •

edited

Loading

shnizzedy left a comment

	orientation = subprocess.run(cmd_3dinfo, capture_output=True, text=True).stdout.strip().upper()
	return orientation

➕ Reorient resources to set orientation in config #2140

➕ Reorient resources to set orientation in config #2140

Conversation

birajstha commented Aug 16, 2024 • edited Loading

Related to

Description

Technical details

Tests

Screenshots

Checklist

Developer Certificate of Origin

shnizzedy left a comment • edited Loading

Choose a reason for hiding this comment

shnizzedy left a comment • edited by birajstha Loading

Choose a reason for hiding this comment

shnizzedy left a comment • edited Loading

Choose a reason for hiding this comment

shnizzedy left a comment

Choose a reason for hiding this comment

birajstha commented Aug 16, 2024 •

edited

Loading

shnizzedy left a comment •

edited

Loading

shnizzedy left a comment •

edited by birajstha

Loading

shnizzedy left a comment •

edited

Loading