-
Notifications
You must be signed in to change notification settings - Fork 112
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
Compressible Euler Quasi-1D #1757
Compressible Euler Quasi-1D #1757
Conversation
Review checklistThis checklist is meant to assist creators of PRs (to let them know what reviewers will typically look for) and reviewers (to guide them in a structured review process). Items do not need to be checked explicitly for a PR to be eligible for merging. Purpose and scope
Code quality
Documentation
Testing
Performance
Verification
Created with ❤️ by the Trixi.jl community. |
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #1757 +/- ##
===========================================
+ Coverage 69.41% 96.23% +26.82%
===========================================
Files 433 436 +3
Lines 34964 35188 +224
===========================================
+ Hits 24270 33862 +9592
+ Misses 10694 1326 -9368
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.
Looks good! I added a few suggestions for clarifying comments, and had one question about the correctness of the cons2entropy
function.
Please also comment with the value of dS/dt
from the entropy conservation test when you get a chance.
Co-authored-by: Jesse Chan <[email protected]>
Co-authored-by: Jesse Chan <[email protected]>
Co-authored-by: Jesse Chan <[email protected]>
Co-authored-by: Jesse Chan <[email protected]>
…shChawla/Trixi.jl into compressible_euler_quasi_1d
The entropy conservation test |
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.
Thanks for the contribution! I left some suggestions and two possible points for discussion.
Please consider also adding consistency checks for the numerical fluxes, see
Lines 628 to 638 in 5c08cf4
@timed_testset "Consistency check for HLL flux (naive): CEE" begin | |
flux_hll = FluxHLL(min_max_speed_naive) | |
# Set up equations and dummy conservative variables state | |
equations = CompressibleEulerEquations1D(1.4) | |
u = SVector(1.1, 2.34, 5.5) | |
orientations = [1] | |
for orientation in orientations | |
@test flux_hll(u, u, orientation, equations) ≈ flux(u, orientation, equations) | |
end |
Thanks for the review, Daniel! |
Co-authored-by: Daniel Doehring <[email protected]>
Co-authored-by: Daniel Doehring <[email protected]>
Co-authored-by: Daniel Doehring <[email protected]>
Co-authored-by: Daniel Doehring <[email protected]>
Co-authored-by: Daniel Doehring <[email protected]>
Co-authored-by: Daniel Doehring <[email protected]>
Co-authored-by: Daniel Doehring <[email protected]>
Co-authored-by: Daniel Doehring <[email protected]>
@KrisshChawla did you get a chance to test if @DanielDoehring's suggestion was faster or slower? |
|
Thanks for checking! The failing tests seem to be related to other test failures (e.g., #1762 and #1759). Let's wait to see if we can figure out why these are failing - they don't have anything to do with these changes though so we shouldn't need to update this PR any more. |
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.
I changed two comments.
Adding quasi 1d compressible Euler equations, as well as an entropy conservative flux for the system.