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

Clean up tutorial notebooks and documentation #213

Merged
merged 17 commits into from
Feb 8, 2024

Conversation

fkuehlein
Copy link
Collaborator

@fkuehlein fkuehlein commented Jan 8, 2024

thereby resolving #185.

In bcc009d I updated the tutorial documentation, directly including rendered views of the notebooks using nbsphinx and nbsphinx-link, I think its quite a neat and cool way to make use of the notebooks. Any objections to these additional dependencies?

Note: nbsphinx currently throws a warning about an outdated pandoc version, which apparently is a known and resolved compatibility issue with nbconvert. Couldn't yet find out why it still throws the warning, but tox is passing anyway.

Also, I noticed the EventSeriesAnalysis tutorial is giving some unexpected results leading to division by zero warnings and such. (edit: resolved as of e19906e)

- delete old tutorials in `examples/tutorials`
- move new tutorials from `notebooks` to `examples/tutorials`
- reorder image and data files in `examples/tutorials`
- update to tqdm progressbar in `RecurrenceNetwork.ipynb`
- avoid pik-copan#210 in `ClimateNetworks.ipynb`
- clean up some typos and latex sequences in all tutorials
- largely resolves pik-copan#185
- add `nbsphinx` and `nbsphinx-link` dependencies to
  directly include new jupyter-notebook tutorials in docs
- minor typo corrections and renames in tutorials
- related: pik-copan#185
- add tutorial on `CoupledClimateNetworks`
- include in docs
@fkuehlein fkuehlein added this to the Release 0.7 milestone Jan 8, 2024
@fkuehlein fkuehlein linked an issue Jan 8, 2024 that may be closed by this pull request
4 tasks
@fkuehlein
Copy link
Collaborator Author

Build is not being started... seems like we've already used up our credits at Travis again? @jdonges, could you try and find out? If so, maybe it would be worth considering GithubActions before we start setting up Windows and macOS builds...

@fkuehlein fkuehlein requested a review from ntfrgl January 8, 2024 11:02
- because original one had lead to trivial results and divide-by-zero warnings.
- also corrected more typos in `EventSeriesAnalysis.ipynb`
- related: pik-copan#185
@fkuehlein
Copy link
Collaborator Author

overwrote the last commit with a force-push of the identical changes, just to trigger a travis build. Seems to work again now as our credits have been re-stocked and we are welcome to ask for more any time!

@fkuehlein
Copy link
Collaborator Author

Build failed but somehow I don't have access to the build logs anymore, so I can't see why. If someone can and has a hint, let me know, otherwise will wait for my account to be granted full access to Travis!

- `nbsphinx` needs `ipython`, therefore add another dependency
Copy link

codecov bot commented Jan 9, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (f1b6e71) 62.82% compared to head (b351d31) 62.82%.

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #213   +/-   ##
=======================================
  Coverage   62.82%   62.82%           
=======================================
  Files          43       43           
  Lines        6220     6220           
=======================================
  Hits         3908     3908           
  Misses       2312     2312           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ntfrgl
Copy link
Member

ntfrgl commented Feb 2, 2024

@fkuehlein, please resolve the conflicts after merging #217. I will provide feedback afterwards; I guess via code comments, since this PR comes from your fork.

- update filepaths
- add `pandoc` dependency necessary for windows build
@fkuehlein
Copy link
Collaborator Author

fkuehlein commented Feb 2, 2024

Builds failed because in 1bd9ab8 I accidentally lost the additional dependencies under docs in the travis.yml. Windows and macOS, however, are reporting not to find pandoc although it is apparently installed. Will try adding it to travis.yml, which will probably not solve it for the Windows build, though. Read about some complications with python pandoc from pip here. @ntfrgl, any ideas on this? If it persists, maybe the notebook-inclusion is not worth all those additional dependencies..

@ntfrgl ntfrgl added the maintenance something should be improved or is outdated label Feb 3, 2024
- on Linux & macOS: via Conda
- on Windows: via Chocolatey
@ntfrgl
Copy link
Member

ntfrgl commented Feb 3, 2024

@fkuehlein, in order to iterate quickly, I would like to be able to push directly to this PR from your fork. This should be possible at first glance (Maintainers are allowed to edit this pull request), but in fact access is denied when I attempt it (remote: error: GH006: Protected branch update failed for refs/heads/master). Please either:

  • temporarily remove the "branch protection rule" for your fork's master branch,
  • open a separate PR with the same commits from a feature branch on your fork, which I should hopefully be able to edit as expected, or
  • open a separate PR with the same commits from a feature branch on this repository.

Regarding the problem in your previous message, I have a candidate solution which installs the Haskell executable pandoc on Windows via choco, while the Python wrapper pandoc remains in the Python dependency list. The conda approach, which we use on Linux and macOS, takes care of both.

@fkuehlein
Copy link
Collaborator Author

hi @ntfrgl, sorry for the late reply, I just removed the branch protection rule on fkuehlein:master. Let me know if you can push now, otherwise I will create a new one!

Package sdist & repository:
- add tutorial sources
- remove generated files
- remove datasets

Code:
- download datasets within notebooks
- reduce redundancy
- group dual pairs of plots
- adjust some parameters
- rerun notebooks

Text & math:
- fix typos, edit lightly, update references
- fix TeX parsing errors in `nbsphinx`
- optimise outline hierarchy for `nbsphinx
Copy link
Member

@ntfrgl ntfrgl 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 merging the various tutorial formats! The additional dependencies are definitely worth it, particularly since they are only in the [docs] extra.

I have done a nontrivial amount of editing and refactoring and also re-executed all notebooks, so I would appreciate a careful review from you. In particular, the nbsphinx parser is somewhat brittle, so one needs to check both output formats for the notebooks.

As a little highlight, the most noticeable change in the tutorial outputs was for the Lorenz system in the recurrence network tutorial, presumably due to our related bug fixes. From what I can tell, the 3D plot of the Lorenz attractor seems to have less artifacts, and the dependence of the transitivity dimension on the recurrence rate threshold looks more reasonable to me.

As you can see, the installation issue with pandoc was resolved as expected. However, there is a little remaining problem only on Python 3.8:

[gw0] node down: Not properly terminated
replacing crashed worker gw0
...
worker 'gw0' crashed while running 'tests/test_climate/test_map_plot.py::TestMapPlot::test_plot'

This problem appeared in two consecutive CI runs, so it seems to be deterministic. Could you please try to replicate it locally? I expect this to be a version-specific issue with one of the libraries pathlib, requests, h5netcdf or cartopy.

- namely cartopy dependency issues on Python 3.8
@fkuehlein
Copy link
Collaborator Author

fkuehlein commented Feb 7, 2024

On my local machine I found the problem to be with cartopy and its dependencies. I tried conda and pip installs of different versions, and finally found having cartopy >= 0.21 with an additional pyproj >= 3.4 in the setup.cfg to work, although throwing a DeprecationWarning (see commit above).

@ntfrgl, I hope this doesn't conflict with your reasoning of adding cartopy <= 0.21 ; python_version < 3.9 in aa39cf3? Also, we should probably document this additional dependency somewhere, for anyone who would want to use MapPlot with Python 3.8.

@ntfrgl
Copy link
Member

ntfrgl commented Feb 8, 2024

Thank you for investigating this! I see, I had unintentionally over-constrained the micro version for cartopy on Python 3.8, which made the build miss SciTools/cartopy#2110. I have now corrected the version constraints to capture the compatibility boundary with Python 3.8 more accurately.

@fkuehlein
Copy link
Collaborator Author

Thank you @ntfrgl for finalising! Also thanks again for reviewing the tutorials with some great cleanups and improvements. Glad to see this PR good to go.

@fkuehlein fkuehlein merged commit 2739894 into pik-copan:master Feb 8, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintenance something should be improved or is outdated
Projects
None yet
Development

Successfully merging this pull request may close these issues.

MAINT: Merge examples and notebooks folder Not compatible with the latest pandoc version
2 participants