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

Make Taal pass Taam tests #267

Merged
merged 146 commits into from
Nov 11, 2020
Merged

Make Taal pass Taam tests #267

merged 146 commits into from
Nov 11, 2020

Conversation

sloede
Copy link
Member

@sloede sloede commented Oct 29, 2020

The idea for this PR is to make Taal pass all Taam tests. That is, we use the same number of analysis nodes in Taal as for Taam, and we temporarily switch to the old time step calculation scheme. Then, we confirm that Taal passes all Taam tests by exactly copying the Taam error norms.

Once we created elixirs for all Taam tests, i.e., formally have at least the same level of coverage (feature-wise) as we did before, we can decide to switch back to the new time step calculation scheme (and possibly also change the number of analysis nodes again).

While this might result in more work (since we have to re-run all tests in the end to get new reference values for Taal after we switch the time step calculation), it gives us the guarantee that Taal produces exactly (up to machine precision) the same results as for Taam for all tests.

To ensure that no test is accidentally left behind, all @testset names in Taal and Taam are prefixed by taal-check-me. If, for a given test, the reference values from Taam are copied to the Taal test file and it is confirmed that the results match, this should be changed to taal-confirmed. In the end, we should only have tests with taal-confirmed in all test files (at least for the parameter file/elixir tests), and can delete it when we switch the time step calculation scheme.

Notes
Code that needs to be updated/adapted after all tests are synchronized is marked by

# FIXME Taal restore after Taam sync

TODO

After action cleanup

  • Apply fixes to MHD's calcflux (c6f4cf4), this will need done for 1D, 2D, and 3D
  • Fix calculation of c_h for MHD

@ranocha
Copy link
Member

ranocha commented Oct 30, 2020

In 2D advection, currently Taal still uses the new time step calculation - but the results still match? Needs investigating...

These use constant velocities like (1.0, 1.0). Hence, the new time step calculation in Taal is equivalent to the old one in Taam if the CFL number is scaled by a factor of two.

@sloede
Copy link
Member Author

sloede commented Oct 30, 2020

In 2D advection, currently Taal still uses the new time step calculation - but the results still match? Needs investigating...

These use constant velocities like (1.0, 1.0). Hence, the new time step calculation in Taal is equivalent to the old one in Taam if the CFL number is scaled by a factor of two.

Thanks, this was the case - I had forgotten to update the CFLs. Fixed in d6ad86f

@ranocha
Copy link
Member

ranocha commented Oct 30, 2020

I've ported the new AMR controller from #266 to dev. You can use them by merging dev into this branch.

@ranocha ranocha linked an issue Nov 10, 2020 that may be closed by this pull request
44 tasks
Comment on lines 54 to 58
@testset "taal-confirmed elixir_advection_timeintegration.jl" begin
test_trixi_include(joinpath(EXAMPLES_DIR, "elixir_advection_timeintegration.jl"),
@test_trixi_include(joinpath(EXAMPLES_DIR, "elixir_advection_timeintegration.jl"),
l2 = [9.144681765639205e-6],
linf = [6.437440532547356e-5])
end
Copy link
Member

Choose a reason for hiding this comment

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

When sorting the tests, I would like to make this test without modifications the first one and check the other time integration methods afterwards.

Copy link
Member Author

Choose a reason for hiding this comment

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

👍

@sloede sloede mentioned this pull request Nov 11, 2020
50 tasks
@ranocha ranocha marked this pull request as ready for review November 11, 2020 06:05
@ranocha ranocha changed the title WIP: Make Taal pass Taam tests Make Taal pass Taam tests Nov 11, 2020
@ranocha ranocha merged commit 406ccc2 into dev Nov 11, 2020
@ranocha ranocha deleted the msl/sync-taal-taam branch November 11, 2020 06:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Reproducibility between Taam and Taal Taal: Porting parameters_*.toml to elixir_*.jl
3 participants