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.

Sign up for GitHub

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

Jump to bottom

ENH: Adding python sample for ConvolveImageWithKernel #376

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

ENH: Adding python sample for ConvolveImageWithKernel #376

wants to merge 1 commit into from

Conversation

PranjalSahu
Copy link
Collaborator

No description provided.

@github-actions github-actions bot added area:Filtering Issues affecting the Filtering module language:Python Changes to Python examples type:Enhancement Improvement of existing methods or implementation labels May 20, 2022
@PranjalSahu PranjalSahu mentioned this pull request May 20, 2022
Open
tbirdso
tbirdso suggested changes May 20, 2022
View reviewed changes
src/Filtering/Convolution/ConvolveImageWithKernel/Code.py Outdated
Comment on lines 29 to 31
import matplotlib.pyplot as plt
plt.imshow(filteredImage)
plt.show()
Copy link
Contributor

Choose a reason for hiding this comment

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

Per discussion, could you please add 1) output with itk.imwrite for baseline comparison against the C++ example, and 2) perhaps a screenshot of matplotlib output?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added matplotlib visualization screenshot.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I will build it locally to see how it renders.

@github-actions github-actions bot added area:Documentation Issues affecting the Documentation module type:Data Changes to example data (usually displayed images) labels May 21, 2022
@github-actions github-actions bot added the type:Infrastructure Infrastructure/ecosystem related changes, such as CMake or buildbots label May 21, 2022
@PranjalSahu PranjalSahu marked this pull request as draft May 21, 2022 22:10
@thewtex
Copy link
Member

thewtex commented May 23, 2022

Hi @PranjalSahu , Could you please rebase on master -- there were package improvements unrelated to your patch that may be causing issues in CI.

@PranjalSahu
Copy link
Collaborator Author

It is failing due to automatically generated test for baseline comparison between C++ and Python
ConvolveImageWithKernelTestBaselineComparisonPython.

@tbirdso
Copy link
Contributor

tbirdso commented May 23, 2022

Ah right, I've also run into issues with C++/Python baseline comparisons before. The ITKSphinxExamples macros were set up with the assumption that C++ and Python script output should always be the same and therefore use the same baseline image; as you've demonstrated here, there are exceptions to this rule.

Some possible paths forward:

  1. Explicitly port the C++ sample to Python, using the exact same data and image pipeline, so that the result is the same
  2. Disable the baseline comparison for now
  3. Move the Python sample to a new example with a PYTHON_ONLY baseline check
  4. Modify the CMake baseline macro so that different baselines can be explicitly provided for C++ and Python examples at the user's discretion

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tbirdso tbirdso tbirdso requested changes

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
area:Documentation Issues affecting the Documentation module area:Filtering Issues affecting the Filtering module 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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@PranjalSahu @thewtex @tbirdso