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

Remove use of MemoizeJac #161

Closed
wants to merge 2 commits into from
Closed

Conversation

nrontsis
Copy link
Contributor

@nrontsis nrontsis commented Dec 9, 2022

Removes use of scipy.optimize.optimize.MemoizeJac because it is not publicly exposed by recent versions of scipy. See #160 for more details. This is done by dropping support for passing jac=True in the scipy interface.

Testing: The jax-scipy example was verified to work.

@moorepants
Copy link
Collaborator

I looked a bit more closely here. See this test:

https://github.com/mechmotum/cyipopt/blob/master/cyipopt/tests/unit/test_scipy_optional.py#L164

If you provide a single function that calculates the objective and its gradient (similarly for the constraints), then you set jac=True and the function is cached so that the function and gradient are never calculated twice (because they are called at least twice for the same args).

The change you have here probably loses that functionality. This isn't well documented, but I think it is correct functionality to use MemoizeJac to avoid the duplicate computations.

@nrontsis
Copy link
Contributor Author

nrontsis commented Dec 9, 2022

Thanks, I agree that if a combined callback is used, then using some of sort of caching makes sense.

But do you think it’s worth maintaining support for combined callbacks at the expense of using an effectively nonpublic scipy import?

@moorepants
Copy link
Collaborator

I've opened a fix in scipy. The class was public at one point and we can see if they'll return it to being public.

@moorepants
Copy link
Collaborator

Thanks for the submission. I'm closing in favor of #183

@moorepants moorepants closed this Feb 12, 2023
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