-
-
Notifications
You must be signed in to change notification settings - Fork 553
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
Make jax optional #1767
Make jax optional #1767
Conversation
f4d6bf3
to
eecc1fe
Compare
60378f1
to
c9e1b59
Compare
Looks good, just a few more tests that need to be skipped if jax is not installed |
tests/unit/test_citations.py
Outdated
system() == "Windows" or (system() == "Darwin" and "ARM64" in version()), | ||
"JAX not supported on windows or Mac M1", | ||
) | ||
@unittest.skipIf(not(importlib.util.find_spec("jax")), "requires jax") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
create a pybamm.have_jax()
function, for example in the jax solver, for this (see
PyBaMM/pybamm/solvers/idaklu_solver.py
Line 20 in 989811b
def have_idaklu(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added the have_jax in util because it was causing circular imports
Also, Jax should be installed at least for the ubuntu tests |
We should also say somewhere (maybe in the readme) which jax version should be installed for this to work. |
Re-running all the tests with and without jax is overkill (adds a lot of time to the CI). I think using the ubuntu tests to test with jax and the windows tests to test without jax is good enough. Otherwise we could specifically run only the jax tests separately, but I don't think that's necessary |
@tinosulzer Which tests are not yet covered |
These two:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good other than that docs error. Perhaps the JaxSolver
class should exist and simply raise an error in __init__
if jax is not installed (so that JaxSolver()
errors but the documentation builds). Alternatively, the simpler option is to install jax when building the docs
Jax is used everywhere in |
58ce6cb
to
674c2bc
Compare
Codecov Report
@@ Coverage Diff @@
## develop #1767 +/- ##
===========================================
- Coverage 99.28% 99.24% -0.05%
===========================================
Files 343 343
Lines 18945 18962 +17
===========================================
+ Hits 18810 18818 +8
- Misses 135 144 +9
Continue to review full report at Codecov.
|
Can you install jax for coverage? |
How do I test |
Windows tests should cover the not-jax case |
005efd6
to
5f7df6e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good now, thanks
Description
Make jax and jaxlib optional
Fixes #1701
Fixes #1775
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.
Key checklist:
$ flake8
$ python run-tests.py --unit
$ cd docs
and then$ make clean; make html
You can run all three at once, using
$ python run-tests.py --quick
.Further checks: