-
Couldn't load subscription status.
- Fork 131
Added abstract vector for max_abs_speed_naive and CompressibleEulerMulticomponentEquations2D. #2607
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
base: main
Are you sure you want to change the base?
Conversation
…leEulerMulticomponentEquations2D.
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 Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2607 +/- ##
=======================================
Coverage 96.81% 96.81%
=======================================
Files 535 536 +1
Lines 42813 42859 +46
=======================================
+ Hits 41446 41492 +46
Misses 1367 1367
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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.
Could you add unit tests that show that the speed for the vectors [1, 0] and [0, 1]matches the existing speed for directions 1, 2?
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
This fixed the bug that the new max_abs_speed was not being used.
|
Still getting some type instability and memory allocations. I am stumped, as any attempted fix does nothing. |
| limiter_idp = SubcellLimiterIDP(equations, basis; | ||
| positivity_variables_cons = ["rho" * string(i) | ||
| for i in eachcomponent(equations)]) | ||
|
|
||
| volume_integral = VolumeIntegralSubcellLimiting(limiter_idp; | ||
| volume_flux_dg = volume_flux, | ||
| volume_flux_fv = surface_flux) |
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.
Did you check this code for type instabilities? Do you get type instabilities with mainstream solvers like shock capturing Hennemann & Gassner?
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.
+1 that is something you should check first
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.
Would that not require a min_max_speed_davis? If so it might just shift the type instability.
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.
Your new code looks reasonable. Thus, I guess the type instabilities come from this choice of solver. You can also profile it or use Cthulhu.jl to @descend into our rhs!.
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.
It seems that the type instability does not come from max_abs_speed_naive after all.
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.
Strangely enough,
@btime Trixi.rhs!($du, $sol.u[1], $semi, 0.0)
does not give me any extra memory allocations.
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
| norm_ = norm(normal_direction) | ||
| normal_vector = normal_direction / norm_ | ||
| # Project velocities onto the direction normal_direction. | ||
| v_ll = dot(v_ll_vec, normal_vector) | ||
| v_rr = dot(v_rr_vec, normal_vector) | ||
|
|
||
| # Compute pressures | ||
| p_ll = (gamma_ll - 1) * (rho_e_ll - 0.5f0 * dot(v_ll_vec, v_ll_vec) * rho_ll) | ||
| p_rr = (gamma_rr - 1) * (rho_e_rr - 0.5f0 * dot(v_rr_vec, v_rr_vec) * rho_rr) | ||
|
|
||
| # Sound speeds | ||
| c_ll = sqrt(gamma_ll * p_ll / rho_ll) | ||
| c_rr = sqrt(gamma_rr * p_rr / rho_rr) | ||
|
|
||
| return (max(abs(v_ll), abs(v_rr)) + max(c_ll, c_rr)) * norm_ |
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 the normal stuff a bit to follow
Trixi.jl/src/equations/compressible_euler_2d.jl
Lines 1416 to 1433 in 1d75f8f
| @inline function max_abs_speed_naive(u_ll, u_rr, normal_direction::AbstractVector, | |
| equations::CompressibleEulerEquations2D) | |
| rho_ll, v1_ll, v2_ll, p_ll = cons2prim(u_ll, equations) | |
| rho_rr, v1_rr, v2_rr, p_rr = cons2prim(u_rr, equations) | |
| # Calculate normal velocities and sound speed | |
| # left | |
| v_ll = (v1_ll * normal_direction[1] | |
| + | |
| v2_ll * normal_direction[2]) | |
| c_ll = sqrt(equations.gamma * p_ll / rho_ll) | |
| # right | |
| v_rr = (v1_rr * normal_direction[1] | |
| + | |
| v2_rr * normal_direction[2]) | |
| c_rr = sqrt(equations.gamma * p_rr / rho_rr) | |
| return max(abs(v_ll), abs(v_rr)) + max(c_ll, c_rr) * norm(normal_direction) |
closer. But better check this for a convergence problem or with unit tests
Co-authored-by: Hendrik Ranocha <[email protected]>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: Hendrik Ranocha <[email protected]>
| @inline function max_abs_speed_naive(u_ll::SVector{N,T}, u_rr::SVector{N,T}, normal_direction::SVector{2,T}, | ||
| equations::CompressibleEulerMulticomponentEquations2D) where {N, T<:AbstractFloat} |
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.
[JuliaFormatter] reported by reviewdog 🐶
| @inline function max_abs_speed_naive(u_ll::SVector{N,T}, u_rr::SVector{N,T}, normal_direction::SVector{2,T}, | |
| equations::CompressibleEulerMulticomponentEquations2D) where {N, T<:AbstractFloat} | |
| @inline function max_abs_speed_naive(u_ll::SVector{N, T}, u_rr::SVector{N, T}, | |
| normal_direction::SVector{2, T}, | |
| equations::CompressibleEulerMulticomponentEquations2D) where { | |
| N, | |
| T <: | |
| AbstractFloat | |
| } |
To run on meshes, like p4est meshes we needed a max_abs_speed_naive function for general vectors/directions.