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

Hot fix: corrected condition in lbfgs #350

Merged
merged 1 commit into from
Dec 9, 2022

Conversation

zaccharieramzi
Copy link
Contributor

@zaccharieramzi zaccharieramzi commented Dec 8, 2022

The feature I had introduced in #323 was failing when the run function was jitted and was a no-op when not because of the following reason:

 ~True == -2  # this is True

Therefore when jitted it was complaining about different types in a condition function, and when not jitted it was equivalent to always being False.

EDIT

Actually I am still running into an error when jitted, so will continue to investigate.

The gist of the error is Abstract tracer value encountered where concrete value is expected, basically doing (not self.stop_if_linesearch_fails | ~state.failed_linesearch) is not allowed because one is a bool and the other is an abstract value.

@zaccharieramzi
Copy link
Contributor Author

With a jnp.logical_or the second issue was resolved

@mblondel
Copy link
Collaborator

mblondel commented Dec 9, 2022

Could you add a non-regression test? Thanks

@zaccharieramzi
Copy link
Contributor Author

Yes just did that: my diagnosis was incorrect it was not jitting the run that was problematic but rather not using implicit diff (which I am not in my use case).
Therefore I just added a case in the rosenbrock test, which fails when I don't have the current fix, and doesn't now.

@mblondel
Copy link
Collaborator

mblondel commented Dec 9, 2022

Actually I am still running into an error when jitted, so will continue to investigate.

Is this one fixed too?

@zaccharieramzi
Copy link
Contributor Author

Yes actually the problem was not coming from it being jitted but from setting implicit_diff to False, I had the wrong diagnosis

@mblondel
Copy link
Collaborator

mblondel commented Dec 9, 2022

Perfect! :) Please squash and I'll merge.

@zaccharieramzi
Copy link
Contributor Author

Done you can merge when all green in CI

@copybara-service copybara-service bot merged commit cd38304 into google:main Dec 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants