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

pybamm.JaxSolver docstring improvement #3994

Merged
merged 17 commits into from
May 31, 2024

Conversation

buddhiwisr
Copy link
Contributor

Description

pybamm.JaxSolver. Arg method is optional. Link to jax.experimental.odeint and format as code. For method=‘BDF’ option summarise method used (use docstrings in jax_bdf_integrate.py) or simply link to the docstrings in jax_bdf_integrate.py

Fixes # (issue)
API Docs - Small fixes suitable for first time contributors - pybamm.JaxSolver

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

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, @buddhiwisr! The docstring does not render properly -

image

Could you please fix this? Thanks!

@agriyakhetarpal
Copy link
Member

agriyakhetarpal commented Apr 11, 2024

Hi, @buddhiwisr, a suggestion: could you also link to JAX's intersphinx inventory in the intersphinx_mapping dictionary in docs/conf.py? It will enable external links for the JAX documentation where jax.experimental.odeint is referenced. I think it should be "jax": ("https://jax.readthedocs.io/en/latest", None).

Copy link

codecov bot commented Apr 11, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.58%. Comparing base (a553fe6) to head (4b8858c).
Report is 3 commits behind head on develop.

Current head 4b8858c differs from pull request most recent head 6a15163

Please upload reports for the commit 6a15163 to get more accurate results.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #3994      +/-   ##
===========================================
+ Coverage    99.55%   99.58%   +0.03%     
===========================================
  Files          288      257      -31     
  Lines        21789    21249     -540     
===========================================
- Hits         21692    21161     -531     
+ Misses          97       88       -9     

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

@arjxn-py
Copy link
Member

@buddhiwisr Are you still on it?

@buddhiwisr
Copy link
Contributor Author

@arjxn-py I am not working on it anymore, afraid.

@agriyakhetarpal
Copy link
Member

Would you like to pick it up from here, @arjxn-py? Though the hackathon has passed, I think these contributions will still be useful

@arjxn-py
Copy link
Member

Would you like to pick it up from here, @arjxn-py? Though the hackathon has passed, I think these contributions will still be useful

Sure, I'd be happy to

Copy link
Contributor

@kratman kratman left a comment

Choose a reason for hiding this comment

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

The formatting pointed out before is still not working correctly:
image

@kratman
Copy link
Contributor

kratman commented May 31, 2024

Docs look correct now, but the example notebooks are failing. This passed right before this, so I am going to try just retriggering it

docs/conf.py Outdated Show resolved Hide resolved
Co-authored-by: Agriya Khetarpal <74401230+agriyakhetarpal@users.noreply.github.com>
@arjxn-py
Copy link
Member

Looks like doctests are failing because of ade96fa

@agriyakhetarpal
Copy link
Member

Looks like doctests are failing because of ade96fa

Makes sense. Jax doesn't provide mappings for stable because it doesn't have multi-versioned docs, it just deploys the latest version. Feel free to revert the change

docs/conf.py Outdated Show resolved Hide resolved
pybamm/solvers/jax_solver.py Outdated Show resolved Hide resolved
pybamm/solvers/jax_solver.py Outdated Show resolved Hide resolved
@agriyakhetarpal
Copy link
Member

pipx run sphobjinv suggest -t 90 -u https://jax.readthedocs.io/en/latest/ odeint suggests that there is no odeint symbol in the Jax codebase, which is why no clickable link shows up. We'll probably need to update the docstring.

@arjxn-py
Copy link
Member

arjxn-py commented May 31, 2024

pipx run sphobjinv suggest -t 90 -u https://jax.readthedocs.io/en/latest/ odeint suggests that there is no odeint symbol in the Jax codebase, which is why no clickable link shows up. We'll probably need to update the docstring.

Just giving the function name instead of direct linking should work, not the perfect solution but better than spending much time on something minor

cc: @kratman

Co-authored-by: Arjun Verma <arjunverma.oc@gmail.com>
pybamm/solvers/jax_solver.py Outdated Show resolved Hide resolved
Co-authored-by: Agriya Khetarpal <74401230+agriyakhetarpal@users.noreply.github.com>
docs/conf.py Outdated Show resolved Hide resolved
Co-authored-by: Eric G. Kratz <kratman@users.noreply.github.com>
@kratman kratman dismissed Saransh-cpp’s stale review May 31, 2024 19:11

The rendering in the original requested change was fixed

@kratman kratman merged commit 16da4ca into pybamm-team:develop May 31, 2024
24 checks passed
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.

None yet

5 participants