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

Temporarily pin autograd < 1.6, fix builds #3062

Conversation

agriyakhetarpal
Copy link
Member

@agriyakhetarpal agriyakhetarpal commented Jun 22, 2023

Description

Adds an upper-bound constraint to autograd's version, in order to fix builds for Windows and ARM M-series macOS, for more details see HIPS/autograd#601

Update: fixed after a 1.6.1 patch release
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.

  • 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

@valentinsulzer
Copy link
Member

We can probably just remove autograd as a dependency, we don't use it anywhere (only in one test specifically just for autograd)

@agriyakhetarpal
Copy link
Member Author

Good idea, since we can fall back to using derivative instead of autograd when differentiating. Do you want me to try doing this in a further PR?

@valentinsulzer
Copy link
Member

Yes please. I don't think the fall back to derivative will work, that is specifically for interpolants. But we never need the fallback in practice

@agriyakhetarpal
Copy link
Member Author

agriyakhetarpal commented Jun 22, 2023

The only place where we use it is here:

def _function_diff(self, children, idx):
"""
Derivative with respect to child number 'idx'.
See :meth:`pybamm.Symbol._diff()`.
"""
# Store differentiated function, needed in case we want to convert to CasADi
if self.derivative == "autograd":
return Function(
autograd.elementwise_grad(self.function, idx),
*children,
differentiated_function=self.function,
)
elif self.derivative == "derivative":
if len(children) > 1:
raise ValueError(
"""
differentiation using '.derivative()' not implemented for functions
with more than one child
"""
)
else:
# keep using "derivative" as derivative
return pybamm.Function(
self.function.derivative(),
*children,
derivative="derivative",
differentiated_function=self.function,
)

Edit: great, opening a PR soon, but I would need some help though

@agriyakhetarpal
Copy link
Member Author

@tinosulzer I thought of some more discussion before I start doing it concretely. Using "derivative" instead of "autograd" did not work, as you said above. Five tests from test_expression_tree.test_functions fail locally when I remove autograd as an option, and it is the error that comes up when len(children) > 1:

ValueError: differentiation using '.derivative()' not implemented for functions with more than one child

should I remove those tests? Debugging this could be slightly outside my forte

@valentinsulzer
Copy link
Member

I see, let's keep this as is then, and just make autograd an optional dependency as is already being done in #3044

@agriyakhetarpal
Copy link
Member Author

Sure, closing this since the build issue was resolved with autograd 1.6.2

@agriyakhetarpal agriyakhetarpal deleted the fix-autograd-build branch June 23, 2023 16:01
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.

2 participants