Skip to content

Conversation

@dzenanz
Copy link
Member

@dzenanz dzenanz commented Jul 7, 2023

This is essentially a migration of KitwareMedical/ITKUltrasound#232 from Ultrasound remote module to here. It involved some non-trivial reorganization. It is also a prettified version of #4099.

PR Checklist

  • No API changes were made (or the changes have been approved)
  • No major design changes were made (or the changes have been approved)
  • Added test (or behavior not changed)
  • Added Python wrapping to new files (if any) as described in ITK Software Guide Section 9.5

@dzenanz dzenanz requested review from tbirdso and thewtex July 7, 2023 14:25
@github-actions github-actions bot added type:Infrastructure Infrastructure/ecosystem related changes, such as CMake or buildbots area:Python wrapping Python bindings for a class type:Testing Ensure that the purpose of a class is met/the results on a wide set of test cases are correct area:Core Issues affecting the Core module area:Filtering Issues affecting the Filtering module area:IO Issues affecting the IO module type:Data Changes to testing data labels Jul 7, 2023
dzenanz added a commit to dzenanz/ITKUltrasound that referenced this pull request Jul 7, 2023
Remove itk::PhasedArray3DSpecialCoordinatesImage wrapping from:
InsightSoftwareConsortium/ITK#4101
Copy link
Contributor

@tbirdso tbirdso left a comment

Choose a reason for hiding this comment

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

Looks great!

@tbirdso
Copy link
Contributor

tbirdso commented Jul 7, 2023

@thewtex Would you please comment on when you expect a v5.4rc01 release to be packaged to include these changes?

Copy link
Member

@thewtex thewtex left a comment

Choose a reason for hiding this comment

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

@dzenanz awesome!!

Could you please check a build of ITKPythonPackage, adjusting the repository and hash here:

https://github.com/InsightSoftwareConsortium/ITKPythonPackage/blob/dc6a18600233ac69a8f42b7489e4edf6a5d8883a/CMakeLists.txt#L90-L93

before merging to ensure that there are not any build errors or wrapping warnings. The package has other wrapping options enabled, and this is a good check for the type combinations enabled here.

@tbirdso ITK 5.4rc01 has been tagged, I am resolving a few wrapping issues. This will be included in 5.3rc02.

@dzenanz
Copy link
Member Author

dzenanz commented Jul 12, 2023

Following the Windows section of wheel building instructions, I invoked building via C:\P\IPP>C:\Python310-x64\python.exe ./scripts/windows_build_wheels.py --py-envs 310-x64 --no-cleanup 1> ../build2.log 2>&1, for the two tags, before and after the changes here (build logs below).

As far as I can tell, these changes did not introduce new errors or warnings. As even the first build did not finish without errors, I am not absolutely certain. Taking a closer look at the logs, there are reassuring entries related to phased array, the last one of which is:

-- Installing: C:/P/IPP/_skbuild/win-amd64-3.10/cmake-install/.//itk/itkPhasedArray3DSpecialCoordinatesImagePython.py

Full build logs:
before.log
after.log

I think we can merge this.

dzenanz added a commit to dzenanz/ITKUltrasound that referenced this pull request Jul 12, 2023
Remove itk::PhasedArray3DSpecialCoordinatesImage wrapping from:
InsightSoftwareConsortium/ITK#4101
@dzenanz
Copy link
Member Author

dzenanz commented Jul 12, 2023

I am building once more locally, I will merge if all goes well.

@dzenanz dzenanz merged commit d564c71 into InsightSoftwareConsortium:master Jul 12, 2023
@dzenanz dzenanz deleted the wrapPA3DSCI branch July 12, 2023 16:22
dzenanz added a commit to dzenanz/ITKUltrasound that referenced this pull request Jul 12, 2023
Remove itk::PhasedArray3DSpecialCoordinatesImage wrapping from:
InsightSoftwareConsortium/ITK#4101
dzenanz added a commit to dzenanz/ITKUltrasound that referenced this pull request Jul 12, 2023
dzenanz added a commit to dzenanz/ITKUltrasound that referenced this pull request Jul 12, 2023
dzenanz added a commit to dzenanz/ITKUltrasound that referenced this pull request Jul 14, 2023
dzenanz added a commit to dzenanz/ITKUltrasound that referenced this pull request Jul 14, 2023
dzenanz added a commit to dzenanz/ITKUltrasound that referenced this pull request Jan 31, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:Core Issues affecting the Core module area:Filtering Issues affecting the Filtering module area:IO Issues affecting the IO module area:Python wrapping Python bindings for a class type:Data Changes to testing data type:Infrastructure Infrastructure/ecosystem related changes, such as CMake or buildbots type:Testing Ensure that the purpose of a class is met/the results on a wide set of test cases are correct

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants