Skip to content

Conversation

@dzenanz
Copy link
Member

@dzenanz dzenanz commented Jul 4, 2023

No description provided.

dzenanz added 4 commits June 30, 2023 15:41
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
@thewtex
Copy link
Collaborator

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