-
Notifications
You must be signed in to change notification settings - Fork 35
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
add retry framework to burn #969
Conversation
this seems to work, although it comes with the overhead of a backup |
Signed-off-by: Michael Zingale <[email protected]>
|
||
// set the Jacobian type | ||
be.jacobian_type = jacobian; | ||
if (!(is_retry && retry_swap_jacobian)) { |
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.
if both of them are false, then wouldn't it still swap the jacobian?
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 you mean is !(false && false) == true
?
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.
indeed, I think we need to rewrite this logic
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.
wait... I think it is okay.
This will evaluate to true is either or both of is_retry
and retry_swap_jacobian
are false, in which case it does not swap the jacobian.
only if both is_retry = true
and retry_swap_jacobian = true
does it do:
be.jacobian_type = (jacobian == 1) ? 2 : 1;
which is the swap.
right?
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 did a quick truth table in a spreadsheet, and the logic is correct right now. It's definitely not obvious at first glance, though. I think swapping the if/else branches would help.
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.
agreed. It wasn't obvious to me either, and I wrote it ;)
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.
updated
This adds the ability to retry the burn in a zone if it fails, without having
to return to the application code.
It provides a few new parameters:
This works well near NSE where sometimes the other Jacobian type can get past
a stubborn burn.
The cost of this change is that we need to store an old copy of the burn_t, but
this is templated out if we don't run with use_burn_retry=1