Skip to content

Remove regrid_on_restart assert and set dt_level in RegridOnly#5456

Open
cityadmirer wants to merge 1 commit into
AMReX-Codes:developmentfrom
cityadmirer:regrid_only
Open

Remove regrid_on_restart assert and set dt_level in RegridOnly#5456
cityadmirer wants to merge 1 commit into
AMReX-Codes:developmentfrom
cityadmirer:regrid_only

Conversation

@cityadmirer

Copy link
Copy Markdown
Contributor

Summary

fix Amr::RegridOnly(): remove assertion and set dt_level

Additional background

Checklist

The proposed changes:

  • fix a bug or incorrect behavior in AMReX
  • add new capabilities to AMReX
  • changes answers in the test suite to more than roundoff level
  • are likely to significantly affect the results of downstream AMReX users
  • include documentation in the code and/or rst files, if appropriate

Amr::RegridOnly(): remove regrid_on_restart assert and set dt_level
```
@WeiqunZhang

Copy link
Copy Markdown
Member

Could you please add some description to the PR? Why is the previous behavior incorrect and how does this PR fix it?

@cityadmirer

Copy link
Copy Markdown
Contributor Author

The full regrid logic is embedded in Amr::timeStep and is complex. So I consider RegridOnly as a light weight, mannual regrid function for arbitrary number of levels, regardless of okToRegrid.
The function is not called by any other codes so regrid_on_restart == 1 assertion is not required.
dt_level should also be updated for data consistency, which resembles the behavior in Amr::timeStep

@WeiqunZhang

Copy link
Copy Markdown
Member

The RegridOnly function is used in some codes like https://github.com/AMReX-Astro/Nyx/blob/699f8cc68d59e0f7786503d1a9b1ea692882bcaa/Source/Driver/nyx_main.cpp#L131 and https://github.com/AMReX-Fluids/IAMR/blob/9bf664bf8e354fb44b75847b62305e6cdaaa890b/Source/main.cpp#L102. The function was probably added for that specific case in mind. I am not saying your change is incorrect or the function's name and current behavior are not confusing. But I am also not 100% sure it will not change the behavior expected by the codes that use it. So I think the safest route is that you add the changes to your code. Note that that does not require you to use your own fork of amrex, because the change can be done in your application code. You can either use the setDtLevel function or you can have your Amr-derived class.

@WeiqunZhang

Copy link
Copy Markdown
Member

We didn't know what to do with your PR #4752 for the similar reason. We are just not confident that it will not break other people's code. Fortunately, you can always derive from amrex::Amr class and customize it to your needs. That was I wanted to suggest in that PR.

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.

2 participants