-
-
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 PyBaMM
importable in Linux
systems if jax
is already installed
#1874
Make PyBaMM
importable in Linux
systems if jax
is already installed
#1874
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #1874 +/- ##
===========================================
+ Coverage 99.26% 99.28% +0.02%
===========================================
Files 345 345
Lines 19107 19106 -1
===========================================
+ Hits 18966 18970 +4
+ Misses 141 136 -5
Continue to review full report at Codecov.
|
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, thanks Saransh! For a more robust fix, would it be possible to check if the jax
version is the correct one and if not, throw a warning and treat the code as if jax
wasn't installed?
I was trying to uninstall the existing version of |
Colab
notebooks workPyBaMM
importable in Linux
systems if jax
is already installed
@@ -356,6 +367,11 @@ def install_jax(): | |||
|
|||
if system() == "Windows": | |||
raise NotImplementedError("Jax is not available on Windows") | |||
elif importlib.util.find_spec("jax") is not None: | |||
if not is_jax_compatible(): | |||
raise ValueError( |
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.
Codecov shows that this complete function is not included in the coverage, should I add # pragma: no cover
for this function?
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.
That sounds good
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.
Also, this is now an error. Should we make it a warning and proceed without 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.
PyBaMM installation (and importing PyBaMM) proceeds without jax
if the installed jax
version is not compatible. But this function throws an error if a user tries to install jax
and has an incompatible jax
version already installed.
Should it instead uninstall the existing version and install the compatible version?
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.
Sorry, I misunderstood! That works as it is :)
We should create a global variable for jax version, jaxlib version and use it in install_jax, is_jax_compatible |
Looks great now, I will wait until tomorrow in case someone else wants to give it a look. |
@all-contributors please add @Saransh-cpp for maintenance |
I've put up a pull request to add @Saransh-cpp! 🎉 |
Description
jax
inhave_jax()
install_jax()
ifjax
is already available with a different versionAdditionally, the cell with
pybamm.print_citations()
gives an error which is described here - pybamm-team/BattBot#47 - and is most probably an error within thepybtex
library.Fixes # (issue)
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: