Skip to content

ENH: Add Jupyter Notebook example for point-set-to-point-set registration#189

Merged
thewtex merged 2 commits into
InsightSoftwareConsortium:masterfrom
tbirdso:point-set-notebook
Feb 25, 2021
Merged

ENH: Add Jupyter Notebook example for point-set-to-point-set registration#189
thewtex merged 2 commits into
InsightSoftwareConsortium:masterfrom
tbirdso:point-set-notebook

Conversation

@tbirdso

@tbirdso tbirdso commented Feb 3, 2021

Copy link
Copy Markdown
Contributor

No description provided.

@tbirdso

tbirdso commented Feb 4, 2021

Copy link
Copy Markdown
Contributor Author

@thewtex I think I solved the entropy graphing issue by truncating initial iteration results outside of the optimizer evaluation window and rounding remaining results as necessary. GradientDescentOptimizerv4Template.GetConvergenceValue() returns the value being checked against the minimum convergence threshold. It's important to note that the value sometimes "bounces" below 0 in a given iteration even though visual inspection shows the point sets are not well aligned. To overcome this for now I've set a sufficiently negative threshold so that the optimizer only exits when it reaches its maximum number of iterations.

JHCT_convergence_metric_eval_rate_each_iteration

I need to read more to understand why the windowed value is sometimes going negative if "convergence" is defined as decreasing below a minimum threshold. @ntustison, do you have a preferred way to show decreasing entropy with the itk::JensenHavrdaCharvatTsallisPointSetToPointSetMetricv4 and itk::GradientDescentOptimizerv4 classes?

@ntustison

Copy link
Copy Markdown
Member

@tbirdso For these types of metrics, which essentially convert the point set distribution to a probability distribution, you need to make sure that the sigma value is sufficiently large. Is that the issue here?

@tbirdso

tbirdso commented Feb 5, 2021

Copy link
Copy Markdown
Contributor Author

Thanks @ntustison, that was indeed the issue. For this example it looks like a point set sigma of at least 20.0 avoids a negative convergence metric (demonstrated below). I'll add a note on this to the notebook.

JHCT_sigma_test

@tbirdso tbirdso force-pushed the point-set-notebook branch 2 times, most recently from 71f73ce to 2f938cc Compare February 17, 2021 18:35
@tbirdso tbirdso changed the title WIP: Add Jupyter Notebook to discuss point-set-to-point-set registration ENH: Add Jupyter Notebook example for point-set-to-point-set registration Feb 17, 2021
@tbirdso tbirdso marked this pull request as ready for review February 18, 2021 14:16

@thewtex thewtex left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Excellent work @tbirdso !!

A few minor issues noted inline.

Also, could you please add a requirements.txt file like with the other notebook examples.

Comment thread src/Registration/Metricsv4/RegisterTwoPointSets/Documentation.rst Outdated
Comment thread src/Registration/Metricsv4/RegisterTwoPointSets/RegisterTwoPointSets.ipynb Outdated
Comment thread src/Registration/Metricsv4/RegisterTwoPointSets/RegisterTwoPointSets.ipynb Outdated
Comment thread src/Registration/Metricsv4/RegisterTwoPointSets/RegisterTwoPointSets.ipynb Outdated
Comment thread src/Registration/Metricsv4/RegisterTwoPointSets/RegisterTwoPointSets.ipynb Outdated
@tbirdso

tbirdso commented Feb 23, 2021

Copy link
Copy Markdown
Contributor Author

Thanks @thewtex, I'll take a look at inline comments. From previous testing it looks like Binder only checks the root-level requirements.txt file, is it sufficient to rely on the requirements.txt previously added in PR 198?

@thewtex

thewtex commented Feb 23, 2021

Copy link
Copy Markdown
Member

@tbirdso when I checked one on the previous PR's with a full path to file notebook in the Binder URL, it used the local folder requirements.txt, if I recall correctly. Have you found this to not be the case?

@tbirdso

tbirdso commented Feb 23, 2021

Copy link
Copy Markdown
Contributor Author

@thewtex It's possible I wasn't using a full notebook path when I checked previously. I will verify and then update this thread.

@tbirdso

tbirdso commented Feb 23, 2021

Copy link
Copy Markdown
Contributor Author

@thewtex After moving requirements.txt from the root directory to the local notebook folder and rebuilding the Binder environment with respect to that folder, it appears that the requirements.txt file is not referenced properly. Binder builds a Docker image with some Python core modules included by default but the notebook cannot be run without additional dependencies such as matplotlib:

ModuleNotFoundError: No module named 'matplotlib'

The Binder landing page describes searching for dependencies at the root:

Binder will search for a dependency file, such as requirements.txt or environment.yml, in the repository's root directory

It doesn't look like there are any Binder examples with subfolders containing either notebooks or requirements.txt files. Note that the MutualInformationAffine Binder example at src/Core/Transform/MutualInformationAffine also does not have a local requirements.txt and relies only on the root requirements.txt file for getting required packages.

Is it sufficient to retain a single requirements.txt at the root and update as necessary? This could create issues of conflicting versions across examples but it seems like Binder links dependencies to the repo level rather than each subfolder. If this becomes an issue we could turn to selectively installing packages with python -m pip install in individual notebooks where necessary.

@thewtex

thewtex commented Feb 24, 2021

Copy link
Copy Markdown
Member

@tbirdso thanks for investigating.

MyBinder is using the repo2docker project for building the Docker images to run the notebooks. Checking the repo2docker documentation, it confirms that it only looks at the root of the repository.

Overall, we make the notebooks self-contained / reproducible, but also enable performant usage of MyBinder and have good testing.

If this becomes an issue we could turn to selectively installing packages with python -m pip install in individual notebooks where necessary.

Yes, this is a good approach, e.g. like we have in the itkwidgets examples.

Towards this end, could you please:

  • Remove requirements.txt from this PR. 🤷‍♂️
  • Add a .binder/requirements.txt file in a PR with common dependencies: itk, matplotlib, itkwidgets. While we should still have a complete set of dependencies in the notebook with pip install in the initial cells, having these pre-installed in the Docker image will speed up that step in MyBinder.
  • Set up nbmake for testing, and ensure we are testing these notebooks.

I will create issues for the latter two.

@tbirdso

tbirdso commented Feb 24, 2021

Copy link
Copy Markdown
Contributor Author

@thewtex I've moved requirements.txt into .binder/ for common dependencies. I will add a separate PR to look into adding nbmake CI procedures for testing notebooks (including those previously merged).

@thewtex thewtex left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks @tbirdso.

For the .binder/requirements.txt file, we only want itk>=5.2rc2, matplotlib, itkwidgets. For the commit prefixes, please use BUG instead of FIX following the prefix guidelines:

https://github.com/InsightSoftwareConsortium/ITK/blob/master/CONTRIBUTING.md#commit-messages

@tbirdso

tbirdso commented Feb 24, 2021

Copy link
Copy Markdown
Contributor Author

Got it, I misunderstood your previous comment. requirements.txt has been updated to reflect only those modules not included by default with repo2docker.

I've squashed commit history to two commits meeting prefix guidelines. The first commit reflects the RegisterTwoPointSets notebook addition while the second solely deals with requirements.txt, which affects all repo Binder examples.

@thewtex thewtex left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

💯

@thewtex thewtex merged commit 063303b into InsightSoftwareConsortium:master Feb 25, 2021
@tbirdso tbirdso deleted the point-set-notebook branch March 17, 2021 16:53
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