optimizer as an argument for optimize#365
Conversation
| calculator = construct_calculator_picklesafe(calculator) | ||
|
|
||
| if smax is None: | ||
| if "precon" in optimizer.__module__.split(".") and smax is None: |
There was a problem hiding this comment.
Would it be safer to do isinstance(optimizer, PreconLBFGS)?
There was a problem hiding this comment.
It seems isinstance is not good way to check it.
optimizer = PreconLBFGS
isinstance(optimizer, PreconLBFGS)
## False
Also there's PreconFIRE. I don't have better idea than this but if you have better solution, please let me know I'll adapt it.
There was a problem hiding this comment.
You're right, isinstance is wrong. Maybe optimizer.__name__ in ["PreconLBFGS", "PreconFIRE"]? Really it would be better to do this by trying to pass smax to the dynamics and falling back if it fails. Let me try to write something like that.
There was a problem hiding this comment.
Could also write it with nested try/except, but I think that ends up uglier
There was a problem hiding this comment.
This is the try/catch version
opt_smax_diff_trycatch.txt
Neither is well tested
There was a problem hiding this comment.
It seems to work for both precon and non-precon optimizers though it's a bit longer.
There seems to be a test failures which is not related to this PR.
There was a problem hiding this comment.
Looks like for some reason gap_fit doesn't exist in the CI. I'll merge this PR, and then try to figure out why that's happening.
|
Thank you for accepting PR ! |
2 participants
As mentioned in #364, I made small change that can take different optimizer for optimize function because some cases, results are quite sensitive to the optimizer.
Hope this might be a useful change.
closes #364