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

Tighten relative tolerance for the testing #341

Merged
merged 6 commits into from
Nov 17, 2020
Merged

Tighten relative tolerance for the testing #341

merged 6 commits into from
Nov 17, 2020

Conversation

andrewwinters5000
Copy link
Member

No description provided.

@codecov
Copy link

codecov bot commented Nov 16, 2020

Codecov Report

Merging #341 (2f25faa) into dev (5bef936) will decrease coverage by 0.02%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##              dev     #341      +/-   ##
==========================================
- Coverage   88.43%   88.40%   -0.03%     
==========================================
  Files          76       76              
  Lines        8886     8886              
==========================================
- Hits         7858     7856       -2     
- Misses       1028     1030       +2     
Flag Coverage Δ
unittests 88.40% <ø> (-0.03%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/callbacks_step/save_solution_dg.jl 90.14% <0.00%> (-2.82%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5bef936...2f25faa. Read the comment docs.

@ranocha
Copy link
Member

ranocha commented Nov 17, 2020

Did you look for CFL magic in the examples where you needed to increase the tolerance?

@andrewwinters5000
Copy link
Member Author

Did you look for CFL magic in the examples where you needed to increase the tolerance?

I plan too for the 1D blast wave because it seems very fishy that switching the shock indicator causes tests to not satisfy rtol=1e-8. It is the same weirdness that all tests with the strict tolerance pass locally but not running on github.

For the parallel I need help from @sloede because I am having troubling running the MPI tests locally.

@sloede
Copy link
Member

sloede commented Nov 17, 2020

I confirmed for the 1D case that it's related to CFL magic. I propose to set the CFL to 0.5 in the elixir and to 0.2 for the two additional tests with density and pressure, then regenerate the references and try again with the regular tolerances. According to my tests, that should probably work (if not, maybe you need 0.4 in the elixir, but the 0.2 is conservative).

Could you do that please, @andrewwinters5000? I will look at the MPI stuff and see if there's an easy way to implement parallel Kahan summation, otherwise I'll just leave the larger tolerances for now.

[edit: reference changed]

@andrewwinters5000
Copy link
Member Author

Could you do that please, @andrewwinters5000?

Yes, I will do this now

@sloede
Copy link
Member

sloede commented Nov 17, 2020

OK, I don't think it is easy enough to implement it in a proper way in parallel that would justify postponing the release of v0.3. Thus if there are no objections from @ranocha, I think this can be merged.

@sloede sloede merged commit a68ee40 into dev Nov 17, 2020
@sloede sloede deleted the tighten_tols branch November 17, 2020 09:08
@ranocha ranocha mentioned this pull request Nov 17, 2020
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