-
Notifications
You must be signed in to change notification settings - Fork 113
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
Sc/polytropic #1526
Sc/polytropic #1526
Conversation
Codecov ReportAll modified lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1526 +/- ##
==========================================
+ Coverage 87.95% 96.09% +8.14%
==========================================
Files 419 423 +4
Lines 34159 34335 +176
==========================================
+ Hits 30043 32992 +2949
+ Misses 4116 1343 -2773
Flags with carried forward coverage won't be shown. Click here to find out more.
☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
overall this looks already very good to me. I have a left only a few minor comments. once resolved, could you please also apply the formatter?
Co-authored-by: Michael Schlottke-Lakemper <[email protected]>
Co-authored-by: Michael Schlottke-Lakemper <[email protected]>
Co-authored-by: Michael Schlottke-Lakemper <[email protected]>
Co-authored-by: Michael Schlottke-Lakemper <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review part 1/2. Second part will follow
examples/structured_2d_dgsem/elixir_euler_polytropic_coupled.jl
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Second part of the review 🙂
Just a comment on the naming convention used for the new system. I think it is misleading to call this new system From what I can tell the goal here is to implement the isothermal Euler equations where Generalizing to polytropic Euler, we take |
Co-authored-by: Michael Schlottke-Lakemper <[email protected]>
Co-authored-by: Michael Schlottke-Lakemper <[email protected]>
…nto sc/polytropic
All tests pass now. We might be ready for merging. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very last comment (sorry, this was an oversight by me earlier 🙈), then ready to merge!
Co-authored-by: Michael Schlottke-Lakemper <[email protected]>
…nto sc/polytropic
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! I just left one small suggestion on a comment in one of the new elixirs.
Polytropic compressible Euler equations in 2d. Can be used to simulate compressible isothermal Euler equations. No viscosity is involved here.
TODO:
Convergence tests using
examples/structured_2d_dgsem/elixir_eulerpolytropic_convergence.jl
:Entropy conservation test using
examples/structured_2d_dgsem/elixir_eulerpolytropic_ec_shockcapturing.jl
: