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

#4183 remove autograd #4196

Merged
merged 11 commits into from
Jun 21, 2024
Merged

#4183 remove autograd #4196

merged 11 commits into from
Jun 21, 2024

Conversation

valentinsulzer
Copy link
Member

Description

Removes autograd dependency and thus enables update to numpy 2.0.
The "differentiate numpy function directly" functionality, which relied on autograd, is deprecated, but I am not aware of any cases where it was being used, since we switched to implementing our own functions and specifying their derivatives.

Fixes #4183

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

@agriyakhetarpal
Copy link
Member

Thanks! Could we unpin NumPy with this? It was recently pinned in #4186

@valentinsulzer valentinsulzer marked this pull request as draft June 18, 2024 16:05
@valentinsulzer
Copy link
Member Author

Converting to draft as the interpolant differentation issue should be fixable

@valentinsulzer
Copy link
Member Author

valentinsulzer commented Jun 20, 2024

Not obvious why test_parameter_values.test_repr is failing, and I'm not able to reproduce it locally.

Edit - looks like it was an issue caused by numpy 2.0, so I have kept numpy pinned to <2.0

Copy link

codecov bot commented Jun 20, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.54%. Comparing base (088ca38) to head (d38117b).
Report is 3 commits behind head on develop.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #4196      +/-   ##
===========================================
- Coverage    99.58%   99.54%   -0.05%     
===========================================
  Files          288      288              
  Lines        21799    21857      +58     
===========================================
+ Hits         21708    21757      +49     
- Misses          91      100       +9     

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

@valentinsulzer valentinsulzer marked this pull request as ready for review June 21, 2024 03:45
Copy link
Sponsor Member

@brosaplanella brosaplanella left a comment

Choose a reason for hiding this comment

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

Looks good, happy to approve once coverage passes (I think some of the deleted tests brought the coverage down).

@agriyakhetarpal
Copy link
Member

The problem with NumPy v2 is with this property that works on self._dict_items for pybamm.Scalar that I remember touching the code for before:

@value.setter
def value(self, value):
    self._value = (
        np.float64(value.item())
        if isinstance(value, np.ndarray)
        else np.float64(value)
    )

which with NumPy v2 returns np.float64(1.602176634e-19) rather than just 1.602176634e-19, i.e., the repr changed for NumPy v2. Could we unpin it and either ignore this error, or just extract the value without conversion to np.float64, or fix the repr in the test?

@valentinsulzer
Copy link
Member Author

I also got this error with numpy 2.0 when trying to run locally:

/Users/vsulzer/Code/PyBaMM/pybamm/expression_tree/symbol.py:10: UserWarning: A NumPy version >=1.22.4 and <1.29.0 is required for this version of SciPy (detected version 2.0.0)
  from scipy.sparse import csr_matrix, issparse

A module that was compiled using NumPy 1.x cannot be run in
NumPy 2.0.0 as it may crash. To support both 1.x and 2.x
versions of NumPy, modules must be compiled with NumPy 2.0.
Some module may need to rebuild instead e.g. with 'pybind11>=2.12'.

If you are a user of the module, the easiest solution will be to
downgrade to 'numpy<2' or try to upgrade the affected module.
We expect that some modules will need time to support NumPy 2.

Traceback (most recent call last):  File "/Users/vsulzer/Code/PyBaMM/examples/scripts/compare_lithium_ion.py", line 4, in <module>
    import pybamm
  File "/Users/vsulzer/Code/PyBaMM/pybamm/__init__.py", line 26, in <module>
    from .expression_tree.symbol import *
  File "/Users/vsulzer/Code/PyBaMM/pybamm/expression_tree/symbol.py", line 10, in <module>
    from scipy.sparse import csr_matrix, issparse
  File "/Users/vsulzer/miniforge3/envs/pybamm/lib/python3.11/site-packages/scipy/sparse/__init__.py", line 295, in <module>
    from ._csr import *
  File "/Users/vsulzer/miniforge3/envs/pybamm/lib/python3.11/site-packages/scipy/sparse/_csr.py", line 11, in <module>
    from ._sparsetools import (csr_tocsc, csr_tobsr, csr_count_blocks,
AttributeError: _ARRAY_API not found
Traceback (most recent call last):
  File "/Users/vsulzer/Code/PyBaMM/examples/scripts/compare_lithium_ion.py", line 4, in <module>
    import pybamm
  File "/Users/vsulzer/Code/PyBaMM/pybamm/__init__.py", line 26, in <module>
    from .expression_tree.symbol import *
  File "/Users/vsulzer/Code/PyBaMM/pybamm/expression_tree/symbol.py", line 10, in <module>
    from scipy.sparse import csr_matrix, issparse
  File "/Users/vsulzer/miniforge3/envs/pybamm/lib/python3.11/site-packages/scipy/sparse/__init__.py", line 295, in <module>
    from ._csr import *
  File "/Users/vsulzer/miniforge3/envs/pybamm/lib/python3.11/site-packages/scipy/sparse/_csr.py", line 11, in <module>
    from ._sparsetools import (csr_tocsc, csr_tobsr, csr_count_blocks,
ImportError: numpy.core.multiarray failed to import

So I think we should leave it pinned and figure out unpinning it properly after the release

@valentinsulzer valentinsulzer merged commit 205ca81 into develop Jun 21, 2024
25 of 26 checks passed
@valentinsulzer valentinsulzer deleted the issue-4183-remove-autograd branch June 21, 2024 14:28
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.

Compatibility with NumPy v2
3 participants