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

add retry framework to burn #969

Merged
merged 38 commits into from
Feb 5, 2024
Merged

Conversation

zingale
Copy link
Member

@zingale zingale commented Aug 22, 2022

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:

  • use_burn_retry
  • retry_swap_jacobians (this will use the other Jacobian type for the retried burn)
  • retry_rtol_spec, retry_rtol_enuc (the relative tolerance to use on the retry)
  • retry_atol_spec, retry_atol_enuc (the absolute tolerance to use on the retry)

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

@zingale
Copy link
Member Author

zingale commented Aug 23, 2022

this seems to work, although it comes with the overhead of a backup burn_t

@zingale
Copy link
Member Author

zingale commented Sep 7, 2022

@zingale zingale changed the title [WIP] add retry framework to burn add retry framework to burn Feb 2, 2024

// set the Jacobian type
be.jacobian_type = jacobian;
if (!(is_retry && retry_swap_jacobian)) {
Copy link
Collaborator

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?

Copy link
Member Author

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?

Copy link
Member Author

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

Copy link
Member Author

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?

Copy link
Collaborator

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.

Copy link
Member Author

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 ;)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated

@zingale zingale merged commit 49fba8d into AMReX-Astro:development Feb 5, 2024
27 checks passed
@zingale zingale deleted the retry_retry branch February 5, 2024 18:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants