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

Add Python wrapping for PhasedArray3DSpecialCoordinatesImage #4101

Merged
merged 2 commits into from
Jul 12, 2023

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 thewtex and tbirdso 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
5 checks passed
@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
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