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

Convert parameter to elixir for 2D Euler tests #229

Merged
merged 19 commits into from
Oct 22, 2020

Conversation

andrewwinters5000
Copy link
Member

Addresses more files in #219

@andrewwinters5000
Copy link
Member Author

I am getting the same weird amr error behaviour as @gregorgassner for #230 . The blob + amr test keeps failing for this reason

@ranocha ranocha requested a review from sloede October 16, 2020 03:59
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.

Looks good in general, thanks!

I just realized that some Euler examples use split_shockcapturing instead of just shockcapturing (what we use for most other examples). Could you please rename the corresponding .toml and .jl files?

@andrewwinters5000
Copy link
Member Author

andrewwinters5000 commented Oct 16, 2020

I just realized that some Euler examples use split_shockcapturing instead of just shockcapturing (what we use for most other examples). Could you please rename the corresponding .toml and .jl files?

Okay, I can rename such cases. Basically because shock capturing => split form in our current form, but future shock capturing strategies may not use the split form. Should I also add split_shockcapturing to mhd blast wave, Orszag Tang and rotor? Also, all the ec tests use the split volume integral, should this be renamed as well?

Update: I realized looking at the naming that the euler + gravity blast wave should also have split_shockcapturing_amr. So the file name should always have initial conditions + all features used, right? Not to be pedantic but shouldn't all the file names have dg in them because that is the particular solver used? I think this needs input from everyone before I make any changes @gregorgassner @sloede @ranocha

@codecov
Copy link

codecov bot commented Oct 16, 2020

Codecov Report

Merging #229 into dev will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##              dev     #229   +/-   ##
=======================================
  Coverage   89.59%   89.59%           
=======================================
  Files          60       60           
  Lines       10485    10485           
=======================================
  Hits         9394     9394           
  Misses       1091     1091           
Impacted Files Coverage Δ
src/Trixi.jl 100.00% <ø> (ø)

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 fc42fd0...a46e23b. Read the comment docs.

@ranocha
Copy link
Member

ranocha commented Oct 17, 2020

Okay, I can rename such cases. Basically because shock capturing => split form in our current form, but future shock capturing strategies may not use the split form. Should I also add split_shockcapturing to mhd blast wave, Orszag Tang and rotor? Also, all the ec tests use the split volume integral, should this be renamed as well?

For me, just using shockcapturing without split is also okay.

Update: I realized looking at the naming that the euler + gravity blast wave should also have split_shockcapturing_amr. So the file name should always have initial conditions + all features used, right?

For me, just using shockcapturing_amr is fine for MHD, too.

Not to be pedantic but shouldn't all the file names have dg in them because that is the particular solver used? I think this needs input from everyone before I make any changes @gregorgassner @sloede @ranocha

I don't think we need dg now - we don't have something else right now so it's okay to postpone this decision until we have a second solver.

@sloede
Copy link
Member

sloede commented Oct 20, 2020

We discussed the naming issue at yesterday's meeting. Our conclusion was that we stick to elixir_equationname_examplename.jl in general. examplename does not have to specify all features that are used, but that it should depend on what the main purpose of this elixir is. If it is an elixir to showcase/validate AMR + shock capturing, elixir_equationname_shockcapturing_amr.jl would be a perfectly suitable name. However, if the main purpose is a specific application that merely happens to use some advanced features, it is not necessary to include them (such as elixir_euler_kelvin_helmholtz_instability.jl or elixir_euler_sedov_blast.jl.

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.

A few minor comments, but the whole looks good to me...

@andrewwinters5000
Copy link
Member Author

Okay, I renamed the files of the elixirs consistently now (I think). @sloede can you double check that I did not miss any? I will update the .toml files too once we settle that all the elixirs are okay...and the tests still pass

@sloede sloede mentioned this pull request Oct 22, 2020
@sloede
Copy link
Member

sloede commented Oct 22, 2020

Okay, I renamed the files of the elixirs consistently now (I think). @sloede can you double check that I did not miss any? I will update the .toml files too once we settle that all the elixirs are okay...and the tests still pass

I created #244 for this to track the discussion on naming the files. I think optimally we should figure out #244 and then put the changes in here, but if your strapped for time, just finish this PR and we'll figure out #244 next week.

@andrewwinters5000
Copy link
Member Author

I created #244 for this to track the discussion on naming the files. I think optimally we should figure out #244 and then put the changes in here, but if your strapped for time, just finish this PR and we'll figure out #244 next week.

I am strapped for time ;) . So should I not worry about the .toml and just merge this now that the tests have passed?

@sloede
Copy link
Member

sloede commented Oct 22, 2020

Yes.

@andrewwinters5000 andrewwinters5000 merged commit f60b3cb into dev Oct 22, 2020
@andrewwinters5000 andrewwinters5000 deleted the convert_remaining_2d_euler branch October 22, 2020 11:09
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