Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update PAM50 template links and fix related test errors #4268

Merged
merged 4 commits into from Oct 25, 2023

Conversation

joshuacwnewton
Copy link
Member

@joshuacwnewton joshuacwnewton commented Oct 24, 2023

Description

This PR updates the PAM50 template links to include new changes to the PAM50 template, including cauda equinea labels (needed for #4250) and updates to the PAM50 cord and levels files. (Full details here.)

These updates, however, caused some failures to existing tests. I previously investigated these in #4254 (comment), and added fixes in #4250 (comment). But, I think it's better to keep all of these related changes outside of the tutorial PR. So, I've cherry-picked the fixes here as well, and will revert the associated commits in #4254.

Linked issues

Fixes #4254.
Partially addresses #4215. (We still need to update the SCT course to account for the spinal levels.)

The results changed because of updates to the `PAM50_cord.nii.gz` file, which affects the registration quality for the DMRI and MT coregistration.
@joshuacwnewton joshuacwnewton added the installation category: install_sct or pip/setup.py label Oct 24, 2023
@joshuacwnewton joshuacwnewton added this to the 6.1 milestone Oct 24, 2023
@joshuacwnewton joshuacwnewton self-assigned this Oct 24, 2023
Copy link
Member

@mguaypaq mguaypaq left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the thorough investigation and the fixes and updates. There's not much to check for the disabled test (I agree with the rationale, and you included a link to it in the comments), and with the updated cached results. I did check that both download links work and point to identical files.

Copy link
Member

@jcohenadad jcohenadad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the investigations @joshuacwnewton ! The changes in results are minor and expected, so I wouldn't worry about this

@joshuacwnewton joshuacwnewton merged commit a5b74ef into master Oct 25, 2023
24 checks passed
@joshuacwnewton joshuacwnewton deleted the jn/4254-update-pam50-dataset branch October 25, 2023 19:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
installation category: install_sct or pip/setup.py
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Recent PAM50 changes have caused test failures
3 participants