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

Add nbqa support support for example notebooks #3110

Merged
merged 7 commits into from
Jul 22, 2023

Conversation

arjxn-py
Copy link
Member

@arjxn-py arjxn-py commented Jul 5, 2023

Description

Adding nbqa as pre-commit hook for jupyter notebooks.

Fixes #2813

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 (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
  • The documentation builds: $ python run-tests.py --doctest

You can run unit and doctests together at once, using $ python run-tests.py --quick.

Further checks:

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

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@Saransh-cpp Saransh-cpp marked this pull request as draft July 5, 2023 21:47
@codecov
Copy link

codecov bot commented Jul 5, 2023

Codecov Report

Patch and project coverage have no change.

Comparison is base (039c786) 99.71% compared to head (d7b7caf) 99.71%.

Additional details and impacted files
@@           Coverage Diff            @@
##           develop    #3110   +/-   ##
========================================
  Coverage    99.71%   99.71%           
========================================
  Files          248      248           
  Lines        18761    18761           
========================================
  Hits         18707    18707           
  Misses          54       54           
Impacted Files Coverage Δ
...bamm/expression_tree/operations/evaluate_python.py 99.29% <ø> (ø)
pybamm/parameters/lithium_ion_parameters.py 100.00% <ø> (ø)
pybamm/solvers/jax_bdf_solver.py 98.89% <ø> (ø)
pybamm/solvers/jax_solver.py 96.77% <ø> (ø)

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@arjxn-py arjxn-py force-pushed the add-nbqa-support branch 2 times, most recently from 8ff3fed to 2431bfb Compare July 12, 2023 10:33
@Saransh-cpp Saransh-cpp marked this pull request as ready for review July 12, 2023 12:35
@arjxn-py
Copy link
Member Author

@brosaplanella in 6-a-simple-SEI-model.ipynb is giving NameError: name 'x_in_metres' is not defined error, can I do something to fix it?

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.

The changes should be structured like this -

  • first commit - adding the new pre-commit, ignoring lines, fixing x_in_metres error
  • second commit - the changes introduced by the new pre-commit
  • third commit - update the .git-blame-ignore-revs with the commit hash of the second commit (and a comment explaining why we are ignoring the commit)

.pre-commit-config.yaml Outdated Show resolved Hide resolved
.pre-commit-config.yaml Outdated Show resolved Hide resolved
@arjxn-py arjxn-py marked this pull request as ready for review July 17, 2023 16:37
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 @arjxn-py! Looks great! Feel free to add more commits to this PR, won't mess with git blame!

.git-blame-ignore-revs Show resolved Hide resolved
.pre-commit-config.yaml Outdated Show resolved Hide resolved
@@ -576,7 +580,7 @@
" ax1.set_ylabel(r'SEI thickness [$\\mu$m]')\n",
" ax1.set_xlabel(r't [$\\mu$s]') \n",
" \n",
" plot_c, = ax2.plot(x * 1e6 * L_out(t_in_seconds), c_out(t_in_seconds, x_in_metres))\n",
" plot_c, = ax2.plot(x * 1e6 * L_out(t_in_seconds), c_out(t_in_seconds, x_in_metres)) # noqa: F821\n",
Copy link
Member

Choose a reason for hiding this comment

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

This is an actual error and should not be ignored. It looks like x_in_metres is not defined in the notebook anywhere and ruff caught that. @brosaplanella might be able to help with this better 🙂

Copy link
Sponsor Member

@brosaplanella brosaplanella Jul 19, 2023

Choose a reason for hiding this comment

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

That is really weird! Locally the notebook doesn't run (both @arjxn-py and I checked) and obviously it shouldn't, as we are calling an undefined variable. @tinosulzer @rtimms do you have any ideas? I have opened #3165 for further discussion.

Co-authored-by: Saransh Chopra <saransh0701@gmail.com>
@brosaplanella
Copy link
Sponsor Member

The failing notebook should now be fixed, so if you merge develop it should work now.

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.

Awesome, thanks @arjxn-py!

@Saransh-cpp Saransh-cpp merged commit f588f20 into pybamm-team:develop Jul 22, 2023
17 of 19 checks passed
@arjxn-py arjxn-py deleted the add-nbqa-support branch July 22, 2023 11:32
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.

adding nbQA support for example notebooks
3 participants