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 wrapping and tests for PhasedArray3DSpecialCoordinatesImage #232

Merged
merged 5 commits into from
Jul 5, 2023

Conversation

dzenanz
Copy link
Member

@dzenanz dzenanz commented Jul 4, 2023

No description provided.

Warnings of style:

C:\Dev\ITKUltrasound-22\Wrapping\Modules\Ultrasound\itkPhasedArray3DSpecialCoordinatesImagePython.cpp(5263,54): warning C4834: discarding return value of function with 'nodiscard' attribute
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.

Code looks good to me. @dzenanz can you confirm that Python tests pass locally on your system?

It looks like itk::PhaseArray3DSpecialCoordinatesImage is implemented in ITK/Core/Common, could you comment on the decision to add wrapping here in ITKUltrasound? Maybe the wrapping for itk::PhaseArray3DSpecialCoordinatesImage and general filters could be moved there and then the wrapping for ITKUltrasound spectral analysis filters could be kept here? Though I am not sure whether it is worth the effort or not.

@dzenanz
Copy link
Member Author

dzenanz commented Jul 5, 2023

All the tests pass locally, including the ones this PR adds.

I also though about adding wrapping to ITK/Core/Common, but:

  1. this class is somewhat specific to ultrasound
  2. releasing a new version of itk-ultrasound is easier and faster than itk, and I need this to continue working on https://github.com/KitwareMedical/SlicerITKUltrasound/tree/pythonization.

Moving the wrapping to ITK core should be easy - just move the .wrap files there. We can discuss with @thewtex whether to do it for ITK 5.4 release.

@tbirdso
Copy link
Contributor

tbirdso commented Jul 5, 2023

@dzenanz Sounds reasonable to me, thanks for commenting. Good to move forward with changes as-is.

@dzenanz dzenanz merged commit 6f6fc8f into KitwareMedical:master Jul 5, 2023
31 checks passed
@thewtex
Copy link
Member

thewtex commented Jul 5, 2023

+1 for migrating this to ITK

@dzenanz
Copy link
Member Author

dzenanz commented Jul 7, 2023

Migrating to ITK via InsightSoftwareConsortium/ITK#4101.

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.

3 participants