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

ENH: ScaleAnImage Python example and input data update #254

Merged
merged 1 commit into from
May 23, 2022

Conversation

tbirdso
Copy link
Contributor

@tbirdso tbirdso commented Mar 17, 2021

  • Adds Python example and baseline test
  • Replaces contrived square image input generated at runtime with static cthead1.png input

Input

ScaleAnImageInput

Output

ScaleAnImageOutputBaseline

@tbirdso tbirdso requested review from thewtex and dzenanz March 17, 2021 20:40
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.

Nice! A few Pythonic improvements noted inline.

src/Core/Transform/ScaleAnImage/Code.py Show resolved Hide resolved
src/Core/Transform/ScaleAnImage/Code.py Outdated Show resolved Hide resolved
src/Core/Transform/ScaleAnImage/Code.py Outdated Show resolved Hide resolved
@tbirdso
Copy link
Contributor Author

tbirdso commented Apr 9, 2021

Seeing a difference in baseline comparisons between cxx and Python examples despite attempting to run the exact same set of procedures for image scaling. Cxx output matches baseline (which was generated with the cxx test) and appears visually correct, Python output does not appear to scale the image border as expected.

I've tested both itk.ResampleImageFilter class directly and the itk.resample_image_filter functional interface, with the same results. @dzenanz do you have any thoughts on possible reasons why we might get different output from similar input to itk::ResampleImageFilter?

Input
ScaleAnImageInput

Cxx output
ScaleAnImageOutput

Python output
ScaleAnImageOutputPython

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.

If float/double difference is not it, I don't know what might be causing it.

src/Core/Transform/ScaleAnImage/Code.py Outdated Show resolved Hide resolved
@tbirdso
Copy link
Contributor Author

tbirdso commented Sep 9, 2021

Closing this stale PR as I haven't been able to pinpoint the issue source and the issue itself seems trivial as it is a difference of one pixel width. Unfortunately there isn't a good way to reference a different baseline image for Python vs cxx tests here.

If this is seen as a symptom of a larger problem (Python vs cxx behavior difference) we can re-submit as an issue against ITK.

@tbirdso tbirdso closed this Sep 9, 2021
@dzenanz dzenanz reopened this Sep 9, 2021
@dzenanz
Copy link
Member

dzenanz commented Sep 9, 2021

An open PR is visible, and people can "steal" code from it easier. If we don't have need/time to resolve the issue now, we should keep it open until we do, or until someone else suggests a fix.

@tbirdso
Copy link
Contributor Author

tbirdso commented Sep 9, 2021

Is it possible this is related to ITK Issue 2703?

Note that the root of discussion there is ResampleInPlaceFilter (docs) and focuses on image direction discrepancies while this example uses ResampleImageFilter and discrepancies in pixel data in the output image. It does not appear that these two filters inherit from each other.

@tbirdso
Copy link
Contributor Author

tbirdso commented Sep 9, 2021

Pinging @brad-t-moore if he is available to get another set of eyes on this issue

@dzenanz
Copy link
Member

dzenanz commented Sep 9, 2021

I don't think this is related to issue 2703.

@github-actions github-actions bot added area:Core Issues affecting the Core module area:Documentation Issues affecting the Documentation module language:C++ Changes to C++ examples language:Python Changes to Python examples type:Data Changes to example data (usually displayed images) type:Enhancement Improvement of existing methods or implementation type:Infrastructure Infrastructure/ecosystem related changes, such as CMake or buildbots labels May 20, 2022
@tbirdso tbirdso changed the title ENH: ScaleAnImage file r/w updates and Python example ENH: ScaleAnImage Python example and input data update May 20, 2022
@tbirdso
Copy link
Contributor Author

tbirdso commented May 20, 2022

@thewtex Ping for re-review

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.

Looks great!

Minor style nitpick inline

src/Core/Transform/ScaleAnImage/Code.py Outdated Show resolved Hide resolved
@tbirdso tbirdso force-pushed the scale-an-image branch 3 times, most recently from 4675e1c to 93d8974 Compare May 20, 2022 20:20
Added `cthead1` input data for example to replace contrived square
image.
@tbirdso
Copy link
Contributor Author

tbirdso commented May 22, 2022

Resolved merge conflicts.

@tbirdso tbirdso merged commit 6d6bb4a into InsightSoftwareConsortium:master May 23, 2022
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:Documentation Issues affecting the Documentation module language:C++ Changes to C++ examples language:Python Changes to Python examples type:Data Changes to example data (usually displayed images) type:Enhancement Improvement of existing methods or implementation type:Infrastructure Infrastructure/ecosystem related changes, such as CMake or buildbots
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants