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: Add an example that demonstrates interfacing with NumPy #366

Merged
merged 13 commits into from
May 23, 2022
Merged

ENH: Add an example that demonstrates interfacing with NumPy #366

merged 13 commits into from
May 23, 2022

Conversation

andinet
Copy link
Collaborator

@andinet andinet commented May 18, 2022

No description provided.

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.

Powered by ReviewNB

@andinet andinet requested a review from tbirdso May 18, 2022 18:19
@github-actions github-actions bot added area:Bridge Issues affecting the Bridge module language:Python Changes to Python examples type:Enhancement Improvement of existing methods or implementation type:Infrastructure Infrastructure/ecosystem related changes, such as CMake or buildbots type:Python Notebook Changes to Python Notebook examples labels May 18, 2022
@github-actions github-actions bot added the area:Documentation Issues affecting the Documentation module label May 18, 2022
tbirdso
tbirdso suggested changes May 19, 2022
View reviewed changes
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.

@andinet The example is looking great! I have requested a couple of CMake changes below.

src/Bridge/CMakeLists.txt Outdated Show resolved Hide resolved
src/Bridge/NumPy/ConvertNumPyArrayToitkImage/CMakeLists.txt Outdated Show resolved Hide resolved
src/Bridge/NumPy/ConvertNumPyArrayToitkImage/Code.py Outdated Show resolved Hide resolved
src/Bridge/NumPy/ConvertNumPyArrayToitkImage/ConvertNumPyArrayToitkImage.ipynb Outdated Show resolved Hide resolved
src/Bridge/NumPy/ConvertNumPyArrayToitkImage/ConvertNumPyArrayToitkImage.ipynb Outdated
],
"source": [
"image = itk.image_from_array(array)\n",
"view(image, cmap='Grayscale', interpolation=False)"
Copy link
Contributor

Choose a reason for hiding this comment

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

When I run the notebook I see a blank background here, do you see the same?

@andinet andinet marked this pull request as draft May 19, 2022 08:54
@github-actions github-actions bot added the type:Data Changes to example data (usually displayed images) label May 19, 2022
@andinet
Copy link
Collaborator Author

andinet commented May 19, 2022 •
edited
Loading

@tbirdso please take a look. I addressed most of your comments.

I still need to update the notebook

tbirdso
tbirdso reviewed May 19, 2022
View reviewed changes
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.

Thank you for the fixes @andinet , the writeups you added look great. Will wait on notebook fixes for final approval.

@andinet
Copy link
Collaborator Author

andinet commented May 20, 2022

@tbirdso

Any thoughts on why the Lint and Documentation tests are failing?

@tbirdso
Copy link
Contributor

tbirdso commented May 20, 2022

@andinet The easiest way to view documentation warnings is on CDash

CDash docs results

It looks like there are style guidelines for the notebook that should be updated. Also, the notebook will need to be added to a docs index, please see other notebook examples for how to do this.

@tbirdso
Copy link
Contributor

tbirdso commented May 20, 2022

2022-05-20T16:30:24.4633969Z
2022-05-20T16:30:24.4634082Z Notebook error:
2022-05-20T16:30:24.4634518Z NoSuchKernel in src/Bridge/NumPy/ConvertNumPyArrayToitkImage/ConvertNumPyArrayToitkImage.ipynb:
2022-05-20T16:30:24.4634961Z No such kernel named python3
2022-05-20T16:30:24.4635272Z ninja: build stopped: subcommand failed.
2022-05-20T16:30:24.4635598Z ninja: build stopped: subcommand failed.
2022-05-20T16:30:24.4635908Z Command exited with the value: 1
2022-05-20T16:30:24.4642646Z Error(s) when building project
2022-05-20T16:30:24.4643474Z MakeCommand:/home/runner/work/_temp/685782585/cmake-3.18.3-Linux-x86_64/bin/cmake --build . --config "Release"
2022-05-20T16:30:24.4644323Z 0 Compiler errors
2022-05-20T16:30:24.4649303Z 1 Compiler warnings
2022-05-20T16:30:24.4649940Z SetCTestConfiguration:BuildDirectory:/home/runner/work/bld/ITKEx-build
2022-05-20T16:30:24.4650387Z SetCTestConfiguration:SourceDirectory:/home/runner/work/Ex

@andinet andinet marked this pull request as ready for review May 20, 2022 23:53
@andinet
Copy link
Collaborator Author

andinet commented May 20, 2022

@tbirdso @thewtex

The one ctest error/warning is still there despite several attempts. I have run out of ideas

@thewtex
Copy link
Member

thewtex commented May 21, 2022

@andinet awesome work, here! 🥇 ✨ ❤️

I was able to reproduce a documentation build error locally -- working on a patch.

thewtex
thewtex approved these changes May 23, 2022
View reviewed 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.

🎇

@thewtex thewtex merged commit 53b7df8 into InsightSoftwareConsortium:master May 23, 2022
@tbirdso
Copy link
Contributor

tbirdso commented May 23, 2022

💯 Thank you for your persistence on this one @andinet and @thewtex !

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

@tbirdso tbirdso tbirdso requested changes

@thewtex thewtex thewtex approved these changes

Assignees

@thewtex thewtex

Labels
area:Bridge Issues affecting the Bridge module area:Documentation Issues affecting the Documentation 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 type:Python Notebook Changes to Python Notebook examples
Projects
None yet
Milestone
No milestone
Development

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