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

ENH: update fsl packages for reported bug fixes #3374

Merged
merged 2 commits into from
Oct 8, 2024

Conversation

psadil
Copy link
Contributor

@psadil psadil commented Oct 8, 2024

Changes proposed in this pull request

Closes #3373

Documentation that should be reviewed

@psadil
Copy link
Contributor Author

psadil commented Oct 8, 2024

FWIW, those failures look like scipy/scipy#21623 (comment)

@effigies
Copy link
Member

effigies commented Oct 8, 2024

Can you look up the latest FSL release and see if the other pins we have are up to date? I'd rather move them as a group, as I expect they are tested together.

@psadil psadil changed the title update fsl-topup for reported bug fixes ENH: update fsl packages for reported bug fixes Oct 8, 2024
@psadil
Copy link
Contributor Author

psadil commented Oct 8, 2024

$ mapfile -t pinned < <(grep fsl- env.yml)
version_pattern="[0-9]+\.[0-9]*"
for fsl_version in 6.0.7.8 6.0.7.9 6.0.7.10 6.0.7.11 6.0.7.12 6.0.7.13; do
    echo "checking FSL ${fsl_version}"
    manifest=$(curl https://git.fmrib.ox.ac.uk/fsl/conda/manifest/-/raw/"${fsl_version}"/fsl-release.yml\?ref_type\=tags 2>/dev/null)    
    for pin in "${pinned[@]}"; do
        package_name=$(echo "${pin}" | grep -oE "fsl-[a-z0-9]+" )
        if [[ $manifest =~ $package_name ]]; then
            manifest_version=$(echo "${manifest}" | grep -oE "${package_name} ${version_pattern}" | grep -oE "${version_pattern}")
            manifest_package_w_version=$(printf "%s=%s" "${package_name}" "${manifest_version}")
            if [[ ! "${pin}" =~ ${manifest_package_w_version} ]]; then
                printf "env has %s but FSL %s has %s\n" "${pin}" "${fsl_version}" "${manifest_package_w_version}"
            fi
        fi
    done
done

checking FSL 6.0.7.8
env has   - fsl-bet2=2111.4 but FSL 6.0.7.8 has fsl-bet2=2111.7
checking FSL 6.0.7.9
env has   - fsl-bet2=2111.4 but FSL 6.0.7.9 has fsl-bet2=2111.7
checking FSL 6.0.7.10
env has   - fsl-bet2=2111.4 but FSL 6.0.7.10 has fsl-bet2=2111.7
checking FSL 6.0.7.11
env has   - fsl-bet2=2111.4 but FSL 6.0.7.11 has fsl-bet2=2111.7
checking FSL 6.0.7.12
env has   - fsl-bet2=2111.4 but FSL 6.0.7.12 has fsl-bet2=2111.8
env has   - fsl-topup=2203.2 but FSL 6.0.7.12 has fsl-topup=2203.4
checking FSL 6.0.7.13
env has   - fsl-bet2=2111.4 but FSL 6.0.7.13 has fsl-bet2=2111.8
env has   - fsl-topup=2203.2 but FSL 6.0.7.13 has fsl-topup=2203.5

Copy link

codecov bot commented Oct 8, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 71.70%. Comparing base (3e69454) to head (69038b7).
Report is 3 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #3374   +/-   ##
=======================================
  Coverage   71.70%   71.70%           
=======================================
  Files          57       57           
  Lines        4259     4259           
  Branches      640      640           
=======================================
  Hits         3054     3054           
  Misses       1090     1090           
  Partials      115      115           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Collaborator

@mgxd mgxd left a comment

Choose a reason for hiding this comment

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

Retriggered the GHA jobs - this looks good to me

@effigies
Copy link
Member

effigies commented Oct 8, 2024

So if I understand correctly, this just means that these lines

https://github.com/nipreps/sdcflows/blob/656a51d4a734d2d8d66550e9e317386f677801a3/sdcflows/workflows/fit/pepolar.py#L144-L145

become unnecessary?

Given that we aren't patching the TOPUP interface directly, this seems fairly safe. @mgxd Were you going to test on a dataset that has an odd number of slices?

@mgxd
Copy link
Collaborator

mgxd commented Oct 8, 2024

No, I don't believe so, the fix for the odd # of slices has to do with a configuration option (--subsamp). We decided against changing the configuration based on even/odd slices (see nipreps/sdcflows#217)

From my understanding this fix will only be relevant/noticed on images with tight FOVs

@mgxd mgxd merged commit 6e6eadc into nipreps:master Oct 8, 2024
18 checks passed
@psadil psadil deleted the enh/update-fsl-topup branch October 8, 2024 22:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update fsl-topup from 2203.2 -> 2203.5?
3 participants