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

Fix calc ch #353

Merged
merged 22 commits into from
Nov 23, 2020
Merged

Fix calc ch #353

merged 22 commits into from
Nov 23, 2020

Conversation

andrewwinters5000
Copy link
Member

@andrewwinters5000 andrewwinters5000 commented Nov 20, 2020

Resolves #257

@ranocha
Copy link
Member

ranocha commented Nov 20, 2020

Feel free to request a review once you're finished with the implementation. GitHub notified me about this new PR. Just a minor comment: If you write something like Closes #257, Fixes #257, or Resolves #257 instead of Addresses issue #257, GitHub will link your PR to the issue and close it once the PR is merged. Thus, you might want to edit the opening post of this PR to make use of this feature.
See https://docs.github.com/en/enterprise/2.16/user/github/managing-your-work-on-github/closing-issues-using-keywords#about-issue-references for further details.

@codecov-io
Copy link

codecov-io commented Nov 20, 2020

Codecov Report

Merging #353 (01b8361) into master (2209514) will decrease coverage by 0.08%.
The diff coverage is 64.70%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #353      +/-   ##
==========================================
- Coverage   90.58%   90.49%   -0.09%     
==========================================
  Files          76       78       +2     
  Lines        8881     8903      +22     
==========================================
+ Hits         8045     8057      +12     
- Misses        836      846      +10     
Flag Coverage Δ
unittests 90.49% <64.70%> (-0.09%) ⬇️

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

Impacted Files Coverage Δ
src/Trixi.jl 100.00% <ø> (ø)
src/callbacks_step/callbacks_step.jl 100.00% <ø> (ø)
src/callbacks_step/stepsize_dg2d.jl 85.18% <ø> (-1.03%) ⬇️
src/callbacks_step/stepsize_dg3d.jl 90.00% <ø> (-0.91%) ⬇️
src/equations/ideal_glm_mhd_2d.jl 92.65% <50.00%> (-0.04%) ⬇️
src/equations/ideal_glm_mhd_3d.jl 94.14% <50.00%> (-0.03%) ⬇️
src/callbacks_step/glm_speed.jl 65.38% <65.38%> (ø)
src/callbacks_step/glm_speed_dg.jl 66.66% <66.66%> (ø)
src/auxiliary/precompile.jl 93.67% <100.00%> (+0.08%) ⬆️
... and 1 more

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 eaa53dd...01b8361. Read the comment docs.

Copy link
Member

@ranocha ranocha left a comment

Choose a reason for hiding this comment

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

Thanks for working on this!

  • You changed the algorithm used to compute c_h. Just to be sure: That was done on purpose and the new way is good, isn't it?
  • The same fix should be implemented in 3D.

src/callbacks_step/glm_speed.jl Outdated Show resolved Hide resolved
src/callbacks_step/glm_speed.jl Outdated Show resolved Hide resolved
src/callbacks_step/glm_speed.jl Outdated Show resolved Hide resolved
src/callbacks_step/glm_speed.jl Outdated Show resolved Hide resolved
src/callbacks_step/glm_speed.jl Outdated Show resolved Hide resolved
src/callbacks_step/glm_speed.jl Outdated Show resolved Hide resolved
examples/2d/elixir_mhd_alfven_wave.jl Outdated Show resolved Hide resolved
src/Trixi.jl Outdated Show resolved Hide resolved
src/Trixi.jl Outdated Show resolved Hide resolved
src/auxiliary/precompile.jl Outdated Show resolved Hide resolved
src/equations/ideal_glm_mhd_2d.jl Outdated Show resolved Hide resolved
src/callbacks_step/glm_speed.jl Outdated Show resolved Hide resolved
src/callbacks_step/glm_speed.jl Show resolved Hide resolved
src/callbacks_step/glm_speed.jl Outdated Show resolved Hide resolved
@andrewwinters5000
Copy link
Member Author

* You changed the algorithm used to compute `c_h`. Just to be sure: That was done on purpose and the new way is good, isn't it?

Yes, now c_h take the fastest magnetoacoustic wave speed into account as well as the geometry of the problem (all done through the time step size)

* The same fix should be implemented in 3D.

Indeed, that is next...

Copy link
Member

@ranocha ranocha left a comment

Choose a reason for hiding this comment

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

LGTM and can be merged after minor revisions indicated below. Thanks for working on this, @andrewwinters5000!

src/callbacks_step/glm_speed.jl Outdated Show resolved Hide resolved
src/callbacks_step/glm_speed.jl Outdated Show resolved Hide resolved
src/callbacks_step/glm_speed_dg.jl Outdated Show resolved Hide resolved
src/callbacks_step/glm_speed_dg.jl Outdated Show resolved Hide resolved
src/callbacks_step/glm_speed.jl Show resolved Hide resolved
src/callbacks_step/glm_speed.jl Outdated Show resolved Hide resolved
src/callbacks_step/glm_speed.jl Show resolved Hide resolved
Copy link
Member

@sloede sloede left a comment

Choose a reason for hiding this comment

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

There are just two minor changes that I think would be helpful to include. Also, I think we should follow @ranocha's suggestion for a modified MHD constructor (for 3D as well of course): https://github.com/trixi-framework/Trixi.jl/pull/353/files#r528295385

src/callbacks_step/glm_speed.jl Outdated Show resolved Hide resolved
src/callbacks_step/glm_speed.jl Outdated Show resolved Hide resolved
Copy link
Member

@sloede sloede left a comment

Choose a reason for hiding this comment

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

LGTM - feel free to merge once all test pass!

Also, congratulations: You are now the second person who ever created a new callback from scratch, which makes you an expert in my eyes ;-)

@andrewwinters5000
Copy link
Member Author

Also, congratulations: You are now the second person who ever created a new callback from scratch, which makes you an expert in my eyes ;-)

Tack så mycket! ;)

@andrewwinters5000 andrewwinters5000 merged commit 79c1a64 into master Nov 23, 2020
@andrewwinters5000 andrewwinters5000 deleted the fix_calc_ch branch November 23, 2020 08:47
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.

Proper computation of GLM divergence cleaning speed
4 participants