-
Notifications
You must be signed in to change notification settings - Fork 56
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
More comprehensive optimiser convergence checks #351
More comprehensive optimiser convergence checks #351
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## v1.4.4 #351 +/- ##
==========================================
- Coverage 97.46% 97.44% -0.03%
==========================================
Files 204 204
Lines 23632 23758 +126
==========================================
+ Hits 23034 23151 +117
- Misses 598 607 +9
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
awesome 🚀
autode/opt/optimisers/base.py
Outdated
|
||
def __post_init__(self): | ||
"""Type checking and sanity checks on parameters""" | ||
self._num_attrs = ["abs_d_e", "rms_g", "max_g", "rms_s", "max_s"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I might make this a property, created based on the attribute type rather than hard-coded strs. I was confused about what this was when I encountered it below
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@t-young31 I made this a property, but still kept hard-coded strs because the attributes may be None at the time of evaluation. I am not really sure how to implement something based on the type without convoluted code.
@t-young31 Could you have one final check before merge please? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👊🏼
Optimiser convergence checks now use ORCA/Gaussian style parameters (energy change, RMS and max gradient and step sizes).
Partial fix of #300
Checklist