-
-
Notifications
You must be signed in to change notification settings - Fork 22
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
Improved Options for Termination Conditions #50
Conversation
Yeah this looks mostly good. It would be good for that number 30 to be tweakable, and add some tests, and I think this is done. |
b38f19b
to
2bb8526
Compare
2bb8526
to
fecfaaa
Compare
fecfaaa
to
a4c2744
Compare
for mode in (:rel_safe, :rel_safe_best, :abs_safe, :abs_safe_best) | ||
T = SteadyStateTerminationCriteria{mode} | ||
mode_val = Val(mode) | ||
@eval function _get_termination_condition(cond::$(T), storage) |
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.
do they need to be dispatches?
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.
It feels wrong to have something that is known at compile time and make them runtime branches 😅
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.
but is this improving runtime or just adding compile time?
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.
This particular set of @eval
s isn't adding any compile time. But the _has_converged
ones add like 0.8ms
over 3.6ms
compilation time.
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 still think it's a hyperoptimization to make this all dispatch, but we can keep it for now.
7dba623
to
d3e23c7
Compare
No description provided.