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

Issue 3328 correctly point notebook download URLs according to versions #3333

Merged

Conversation

agriyakhetarpal
Copy link
Member

Description

Closes #3328; ensures that users of the example notebooks can download the correct notebook from GitHub according to the latest version, stable versions starting from 23.5, and on PR builds. The Google Colab badge now points to the correct renditions of the notebooks, rather than the develop branch.

Type of change

Please add a line in the relevant section of CHANGELOG.md to document the change (include PR #) - note reverse order of PR #s. If necessary, also add to the list of breaking changes.

  • New feature (non-breaking change which adds functionality)
  • Optimization (back-end change that speeds up the code)
  • Bug fix (non-breaking change which fixes an issue)

Key checklist:

  • No style issues: $ pre-commit run (or $ nox -s pre-commit) (see CONTRIBUTING.md for how to set this up to run automatically when committing locally, in just two lines of code)
  • All tests pass: $ python run-tests.py --all (or $ nox -s tests)
  • The documentation builds: $ python run-tests.py --doctest (or $ nox -s doctests)

You can run integration tests, unit tests, and doctests together at once, using $ python run-tests.py --quick (or $ nox -s quick).

Further checks:

  • Code is commented, particularly in hard-to-understand areas
  • Tests added that prove fix is effective or that feature works

@codecov
Copy link

codecov bot commented Sep 14, 2023

Codecov Report

Patch coverage has no change and project coverage change: -0.16% ⚠️

Comparison is base (a47b6c2) 99.70% compared to head (de737dd) 99.55%.
Report is 54 commits behind head on develop.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #3333      +/-   ##
===========================================
- Coverage    99.70%   99.55%   -0.16%     
===========================================
  Files          248      253       +5     
  Lines        18899    19523     +624     
===========================================
+ Hits         18844    19436     +592     
- Misses          55       87      +32     

see 23 files with indirect coverage changes

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

@kratman
Copy link
Contributor

kratman commented Sep 14, 2023

Switching the download URL based on the current version of the docs page sounds good to me.

Will https://docs.pybamm.org default to stable after this change? I know it the previous version the pybamm docs defaulted to latest, which could make the problem repeat.

@agriyakhetarpal
Copy link
Member Author

agriyakhetarpal commented Sep 14, 2023

Will https://docs.pybamm.org/ default to stable after this change?

I just checked the Read the Docs configuration options; I can't find a way to set the stable version as the default one instead of latest when pointing to https://docs.pybamm.org. However, we have been wanting to correct this by being more explicit about it. We initially wanted to implement a version warning banner/bar on the latest version to point users to the stable documentation, but we decided against it because we would not get it for older versions in that case which would be pointless (it cannot be done unless there is a server-side implementation for it). Read the Docs has disabled their version warning bar lately in favour of some work going on at https://github.com/readthedocs/addons that I will look to add as soon as it is available for users.

I know it the previous version the pybamm docs defaulted to latest, which could make the problem repeat.

I get that – the versioning is deemed incorrect for notebooks on 23.5, sadly there is not much that can be done to fix it as of now because the tag already exists. I am hoping this PR can fix it for further versions/releases at least

@kratman
Copy link
Contributor

kratman commented Sep 14, 2023

Ok thank you for taking a look and this is definitely an improvement. I only asked about stable/latest because I had assumed I installed my PyBaMM environment incorrectly when I first saw the errors. However, a release would also prevent that since I assume major changes to parameter and external function names are rare. So this change extra on top of what I was hoping to see.

Copy link
Member

@Saransh-cpp Saransh-cpp left a comment

Choose a reason for hiding this comment

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

Thanks, @agriyakhetarpal! Looks good overall, some comments below.

docs/conf.py Outdated Show resolved Hide resolved
docs/conf.py Outdated Show resolved Hide resolved
docs/conf.py Outdated Show resolved Hide resolved
docs/conf.py Outdated Show resolved Hide resolved
docs/conf.py Show resolved Hide resolved
Copy link
Member

@Saransh-cpp Saransh-cpp left a comment

Choose a reason for hiding this comment

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

Thanks for fixing the mess 🙂

Let me know if this is ready to merge

@agriyakhetarpal
Copy link
Member Author

Yes, please go ahead!

@Saransh-cpp Saransh-cpp merged commit 6ecbd6a into pybamm-team:develop Sep 14, 2023
30 of 32 checks passed
@agriyakhetarpal agriyakhetarpal deleted the notebook-downloads-fix branch September 15, 2023 01:10
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.

Released version of PyBaMM does not match tutorials
3 participants