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

geodesic notebook #277

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

thewtex
Copy link
Member

@thewtex thewtex commented May 14, 2021

  • STYLE: GeodesicActiveContour CMake style
  • DOC: Add notebook example for SegmentWithGeodesicActiveContourLevelSet

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@@ -0,0 +1,262 @@
{
Copy link
Member

Choose a reason for hiding this comment

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

Point picker for itkwidgets would come in handy here for interactive experimentation.


Reply via ReviewNB

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, on the agenda :-).

@dzenanz
Copy link
Member

dzenanz commented May 14, 2021

Is there an already existing example of how a Jupyter notebook looks like when rendered in sphinx examples?

@thewtex
Copy link
Member Author

thewtex commented May 14, 2021

Is there an already existing example of how a Jupyter notebook looks like when rendered in sphinx examples?

https://itk.org/ITKExamples/src/Registration/Metricsv4/RegisterTwoPointSets/Documentation.html

Copy link
Member

@jhlegarreta jhlegarreta left a comment

Choose a reason for hiding this comment

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

Have not tried, but looks good. 💯 for doing this !

@jhlegarreta
Copy link
Member

jhlegarreta commented May 14, 2021

A minor comment: maybe the Jupyter notebook execution-related cell data (e.g. execution_count) can be cleared before merging? Not sure if this has been in other notebooks. nbconvert can do this automatically if I'm not mistaken,

jupyter nbconvert --ClearOutputPreprocessor.enabled=True --clear-output *.ipynb

but it would also clear the output images, if any, I think. Maybe the SimpleITK folks know better how to do this.

At term, maybe having a pre-commit hook taking care of this cleaning could be handy.

@thewtex
Copy link
Member Author

thewtex commented May 17, 2021

@jhlegarreta good ideas.

I will remove the cell output for this notebook, but we will likely want to add it back in the future when itkwidgets saves a more meaningful visualization. Our other notebooks have matplotlib output, which is useful to have saved in the notebook output when browsing on GitHub or nbviewer.

@tbirdso
Copy link
Contributor

tbirdso commented May 18, 2021

Looks great! Notebook is helpful for following each step.

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.

4 participants