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.

Sign up for GitHub

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

Jump to bottom

ENH: Bump for ITK v5.4rc02 #260

Merged
merged 3 commits into from
Nov 1, 2023
Merged

ENH: Bump for ITK v5.4rc02 #260

merged 3 commits into from
Nov 1, 2023

Conversation

tbirdso
Copy link
Collaborator

@tbirdso tbirdso commented Nov 1, 2023

Succeeds #256 to bump ITKElastix for ITK v5.4rc02.

See #256 (comment) for discussion of relevant fixes between ITK v5.4rc01 and v5.4rc02.

Updates CI and bumps itk-elastix to 0.19.1.

N-Dekker and others added 2 commits October 23, 2023 17:54
@N-Dekker
ENH: Require at least ITK 5.4rc2, for ImageRandomIteratorWithIndex
75f3851 
Especially to avoid potential differences between registration results of the
elastix executable and ITKElastix, which were accidentally introduced with ITK
5.4rc1 and fixed with ITK 5.4rc2:

  pull request InsightSoftwareConsortium/ITK#4183
  commit InsightSoftwareConsortium/ITK@997ff54
  BUG: ImageRandomIteratorWithIndex should not assign data in constructor
@tbirdso
ENH: Bump to v0.19.1 for ITK v5.4rc2 pre-release
4e38131 
Bumps `itk-elastix` package version to v0.19.1 and updates CI for the
ITK v5.4rc2 workflow.
@tbirdso tbirdso requested review from dzenanz and N-Dekker November 1, 2023 18:04
@tbirdso tbirdso mentioned this pull request Nov 1, 2023
Closed
dzenanz
dzenanz approved these changes Nov 1, 2023
View reviewed changes
Copy link
Member

@dzenanz dzenanz left a comment

Choose a reason for hiding this comment

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

Looks good, assuming it works (not segfaults).

@tbirdso
Copy link
Collaborator Author

tbirdso commented Nov 1, 2023 •
edited
Loading

Looks good, assuming it works (not segfaults).

Agreed. ITKElastix notebook tests were previously able to detect segfaults, we should verify that they run successfully here.

EDIT: I forgot that ITKElastix uses the latest release instead of build artifacts in its notebook tests. Let me see if it is reasonable to update that behavior here. (xref https://github.com/InsightSoftwareConsortium/ITKElastix/blob/main/.github/workflows/notebook-test.yml)

@tbirdso
ENH: Test notebooks with ITK reusable workflow
ebedbf7 
Replace ITKElastix notebook test workflow with general notebook test
workflow from ITKRemoteModuleBuildTestPackageAction.

Under previous behavior ITKElastix notebook tests relied on the latest
published release and were not reflective of fixes in individual PRs.
The updated workflow uses the latest wheel build artifact in notebook
tests to immediately reflect proposed changes.
@tbirdso
Copy link
Collaborator Author

tbirdso commented Nov 1, 2023

It looks like some notebooks ran registration successfully and the notebooks that did fail were not due to segfaults, which is promising. I will push an update to resolve ITKElastix notebook requirements.

@tbirdso
Copy link
Collaborator Author

tbirdso commented Nov 1, 2023

On further review it looks like a resolution may lie in ITKRemoteModuleBuildTestPackageAction where the binder requirements.txt is not getting picked up correctly. I am in favor of moving forward with this merge and documenting the regression in CI so that we can move forward with releasing v5.4rc2 packages. I should be able to follow up with a proper CI fix in the next day or two.

@dzenanz Do you approve of that plan?

@dzenanz
Copy link
Member

dzenanz commented Nov 1, 2023

If the local testing shows that #258 is fixed, that plan sounds reasonable.

@tbirdso
Copy link
Collaborator Author

tbirdso commented Nov 1, 2023

If the local testing shows that #258 is fixed, that plan sounds reasonable.

Sounds good. I will do more local testing with the build artifacts to confirm before moving forward.

@tbirdso
Copy link
Collaborator Author

tbirdso commented Nov 1, 2023

Confirmed that I am able to reproduce the original segfault issue with itk==v5.4rc02,itk-elastix==0.19.0 on Windows and no longer see the segfault issue with itk==v5.4rc02,itk-elastix==0.19.1 (build artifact). The GroupwiseRegistration notebook test also passes now with this build update. Will move forward with merge/tag/release and revisit test failures soon.

@tbirdso tbirdso merged commit c4a181a into main Nov 1, 2023
30 of 32 checks passed
@tbirdso tbirdso mentioned this pull request Nov 1, 2023
Closed
@thewtex thewtex deleted the bump-5.4rc2 branch November 3, 2023 21:02
N-Dekker added a commit to SuperElastix/elastix that referenced this pull request Nov 6, 2023
@N-Dekker
ENH: Upgrade ITK from v5.3.0 to v5.4rc02 (C++17)
49f64bb 
Upgraded ITK on the CI to version 5.4rc02: https://github.com/InsightSoftwareConsortium/ITK/releases/tag/v5.4rc02

Follow-up to pull request #762 commit 353a095 "COMP: Upgrade ITK from v5.3rc04 to v5.3.0", August 25, 2022.

Following ITKElastix pull request InsightSoftwareConsortium/ITKElastix#260 (merged on November 1, 2023).

ITK v5.4 includes the upgrade from C++14 to C++17.
@N-Dekker N-Dekker mentioned this pull request Nov 6, 2023
Merged
N-Dekker added a commit to SuperElastix/elastix that referenced this pull request Nov 6, 2023
@N-Dekker
ENH: Upgrade ITK from v5.3.0 to v5.4rc02 (C++17)
5f0940d 
Upgraded ITK on the CI to version 5.4rc02: https://github.com/InsightSoftwareConsortium/ITK/releases/tag/v5.4rc02

Follow-up to pull request #762 commit 353a095 "COMP: Upgrade ITK from v5.3rc04 to v5.3.0", August 25, 2022.

Following ITKElastix pull request InsightSoftwareConsortium/ITKElastix#260 (merged on November 1, 2023).

ITK v5.4 includes the upgrade from C++14 to C++17.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dzenanz dzenanz dzenanz approved these changes

@N-Dekker N-Dekker Awaiting requested review from N-Dekker

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@tbirdso @dzenanz @N-Dekker