Use `VectorOfArray` in `wrap_array` for `DGMulti` solvers #2150

jlchan · 2024-11-07T15:29:42Z

@huiyuxie FYI this PR goes towards addressing #1789.

d

github-actions · 2024-11-07T15:30:00Z

Review checklist

This 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

The PR has a single goal that is clear from the PR title and/or description.
All code changes represent a single set of modifications that logically belong together.
No more than 500 lines of code are changed or there is no obvious way to split the PR into multiple PRs.

Code quality

The code can be understood easily.
Newly introduced names for variables etc. are self-descriptive and consistent with existing naming conventions.
There are no redundancies that can be removed by simple modularization/refactoring.
There are no leftover debug statements or commented code sections.
The code adheres to our conventions and style guide, and to the Julia guidelines.

Documentation

New functions and types are documented with a docstring or top-level comment.
Relevant publications are referenced in docstrings (see example for formatting).
Inline comments are used to document longer or unusual code sections.
Comments describe intent ("why?") and not just functionality ("what?").
If the PR introduces a significant change or new feature, it is documented in NEWS.md with its PR number.

Testing

The PR passes all tests.
New or modified lines of code are covered by tests.
New or modified tests run in less then 10 seconds.

Performance

There are no type instabilities or memory allocations in performance-critical parts.
If the PR intent is to improve performance, before/after time measurements are posted in the PR.

Verification

The correctness of the code was verified using appropriate tests.
If new equations/methods are added, a convergence test has been run and the results
are posted in the PR.

Created with ❤️ by the Trixi.jl community.

codecov · 2024-11-07T16:12:17Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.87%. Comparing base (d107764) to head (536f6a7).
Report is 1 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #2150   +/-   ##
=======================================
  Coverage   96.87%   96.87%           
=======================================
  Files         490      490           
  Lines       39458    39462    +4     
=======================================
+ Hits        38224    38228    +4     
  Misses       1234     1234

Flag	Coverage Δ
unittests	`96.87% <100.00%> (+<0.01%)`	⬆️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

jlchan · 2024-11-07T23:09:17Z

@DanielDoehring if I remember correctly, you implemented @allocated tests, right?

I am running into some @allocated failures (for example https://github.com/trixi-framework/Trixi.jl/actions/runs/11732466598/job/32684751742?pr=2150)

elixir_euler_weakform.jl (SBP, EC): Test Failed at /home/runner/work/Trixi.jl/Trixi.jl/test/test_threaded.jl:388
  Expression: #= /home/runner/work/Trixi.jl/Trixi.jl/test/test_threaded.jl:388 =# @allocated(Trixi.rhs!(du_ode, u_ode, semi, t)) < 5000
   Evaluated: 118704 < 5000

However, at least locally, this appears to only happen the first time Trixi.rhs! is run. I tried running Trixi.rhs! to avoid these allocations in CI, but it doesn't seem to work.

Did you run into this issue before?

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> Update test/test_dgmulti_2d.jl Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Apply suggestions from code review Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

…ework/Trixi.jl into jc/wrap_VectorOfArray

DanielDoehring · 2024-11-08T07:36:13Z

However, at least locally, this appears to only happen the first time Trixi.rhs! is run. I tried running Trixi.rhs! to avoid these allocations in CI, but it doesn't seem to work.

Did you run into this issue before?

Hm, I have not encountered this. Can you maybe track down the allocations using the SummaryCallback ? I guess there must be some type instability. But I have no idea why the behaviour after executing rhs! once is different depending on the machine.

jlchan · 2024-11-08T21:07:48Z

Hm, I have not encountered this. Can you maybe track down the allocations using the SummaryCallback ? I guess there must be some type instability. But I have no idea why the behaviour after executing rhs! once is different depending on the machine.

There doesn't seem to be a type instability. There's a weird GC call stack that @profview_allocs shows, I have no idea what's going on. I'll bring it up at the next Trixi meeting.

DanielDoehring · 2024-11-09T12:15:00Z

There doesn't seem to be a type instability. There's a weird GC call stack that @profview_allocs shows, I have no idea what's going on. I'll bring it up at the next Trixi meeting.

Oh wow, that sounds strange. So the allocations are only present in the test "environment"?

jlchan · 2024-11-10T16:05:22Z

There doesn't seem to be a type instability. There's a weird GC call stack that @profview_allocs shows, I have no idea what's going on. I'll bring it up at the next Trixi meeting.

Oh wow, that sounds strange. So the allocations are only present in the test "environment"?

Not exactly - on my machine, the allocations disappear after running rhs! once. In the test environment (or on CI, I can't tell) this isn't the case.

Here's the output from Profile.Allocs.@profile sample_rate=0.001 Trixi.rhs!(du, ode.u0, semi, 0.0) printed to a txt file: profile_allocs.txt.

Project.toml

src/auxiliary/precompile.jl

test/test_dgmulti_1d.jl

huiyuxie · 2024-11-11T03:12:23Z

https://github.com/JuliaLang/julia/blob/3318941e585db632423366b8b703ea55a6ba8421/base/timing.jl#L479-L489
If the allocation is less in the second call compared to the first during a single session, the GC might have been triggered between these two gc_bytes calls. But I also suspect it could be caused by type instability introduced by RecursiveArrayTools.jl. Why do you think it doesn’t seem to be a type instability issue?

huiyuxie · 2024-11-11T03:12:48Z

Here's the output from Profile.Allocs.@Profile sample_rate=0.001 Trixi.rhs!(du, ode.u0, semi, 0.0) printed to a txt file: profile_allocs.txt.

The text file only shows half of the information on each line. Could you provide it in a different way?

jlchan · 2024-11-11T03:37:27Z

Why do you think it doesn’t seem to be a type instability issue?

I checked @code_warntype on the first call; nothing is red and I don't see an Any instance anywhere. If you have any suggestions for where else to check, please let me know.

The text file only shows half of the information on each line. Could you provide it in a different way?

Thanks - how is this?
profile_allocs.txt

huiyuxie · 2024-11-11T03:41:38Z

Same issue - how about telling me how to reproduce it? Use your latest commit and then run Profile.Allocs.@profile sample_rate=0.001 Trixi.rhs!(du, ode.u0, semi, 0.0)?

examples/dgmulti_1d/elixir_euler_shu_osher_gauss_shock_capturing.jl

JoshuaLampert

CI passes 🥳
Here is another round of questions. Hopefully, this is close to be merged now.

Project.toml

src/auxiliary/precompile.jl

test/test_parabolic_2d.jl

jlchan · 2025-02-06T19:11:00Z

@JoshuaLampert thanks so much for fixing the shock capturing test and getting the rest of CI working!

I'm not sure why the behavior of the shock capturing test is so sensitive. Just to clarify - did running with RDPK3SpFSAL49, fixed time-step and CFL control fail CI?

JoshuaLampert · 2025-02-06T19:37:00Z

Just to clarify - did running with RDPK3SpFSAL49, fixed time-step and CFL control fail CI?

For RDPK3SpFSAL49, you need a very small CFL number (aroundcfl = 0.07) or a very small fixed time step (around dt = 0.0006). With SSPRK43 it can be much higher.

test/test_parabolic_3d.jl

Co-authored-by: Joshua Lampert <[email protected]>

ranocha · 2025-02-07T07:34:11Z

docs/src/visualization.md

@@ -174,7 +174,7 @@ julia> compute_vorticity(velocity, semi) =
         compute_vorticity(velocity, Trixi.mesh_equations_solver_cache(semi)...);

 julia> function get_velocity(sol)
-         rho, rhou, rhov, E = StructArrays.components(sol.u[end])
+         rho, rhou, rhov, E = StructArrays.components(Base.parent(sol.u[end]))


This is a breaking PR since we changed documented behavior. I will mark it accordingly and take care of it.

jlchan added 2 commits November 7, 2024 09:28

update bound on RecursiveArrayTools to have access to 3.27.3

3b02770

d

comment out broken precompile statements

9b21e12

jlchan and others added 4 commits November 7, 2024 16:31

adding VoA for DGMulti

e66bcd9

Apply suggestions from code review

8da6bfa

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

Merge branch 'main' into jc/wrap_VectorOfArray

d997c8f

run Trixi.rhs! twice to try to avoid excessive allocations

a20dc52

jlchan and others added 9 commits November 7, 2024 17:17

bump lower compat of RecursiveArrayTools

781a7d8

run Trixi.rhs! twice to reduce allocation count

1d04471

update l2, linf errors

c10b421

Apply suggestions from code review

88d08a1

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

Update test/test_dgmulti_2d.jl

595a75e

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

Apply suggestions from code review

b5c218d

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

Merge branch 'jc/wrap_VectorOfArray' of https://github.com/trixi-fram…

fea32d2

…ework/Trixi.jl into jc/wrap_VectorOfArray

bump LinearAlgebra lower compat for Downgrade CI

d69dbc7

jlchan added 2 commits November 8, 2024 09:28

Merge branch 'main' into jc/wrap_VectorOfArray

823caf1

Merge branch 'main' into jc/wrap_VectorOfArray

bd16969

Merge branch 'main' into jc/wrap_VectorOfArray

0ab4c2e

huiyuxie reviewed Nov 11, 2024

View reviewed changes

Project.toml Outdated Show resolved Hide resolved

src/auxiliary/precompile.jl Outdated Show resolved Hide resolved

test/test_dgmulti_1d.jl Outdated Show resolved Hide resolved

github-actions bot reviewed Feb 6, 2025

View reviewed changes

examples/dgmulti_1d/elixir_euler_shu_osher_gauss_shock_capturing.jl Outdated Show resolved Hide resolved

JoshuaLampert and others added 4 commits February 6, 2025 16:12

format

9cf54fc

fix docs

11bb917

fix test values (use the ones I get locally)

898592f

Keep mu as a constant instead of a function

4cd807f

JoshuaLampert reviewed Feb 6, 2025

View reviewed changes

Project.toml Outdated Show resolved Hide resolved

src/auxiliary/precompile.jl Outdated Show resolved Hide resolved

test/test_parabolic_2d.jl Outdated Show resolved Hide resolved

JoshuaLampert added 4 commits February 6, 2025 17:23

try to fix precompiling of summary callback

1ac1d41

fix precompile

e38c127

try compat "1" for LinearAlgebra

83c2892

revert precompile stuff

a0d96af

JoshuaLampert mentioned this pull request Feb 6, 2025

Bump compat for DiffEqBase.jl and DiffEqCallbacks.jl #2263

Closed

jlchan marked this pull request as ready for review February 6, 2025 19:07

remove #= FIXME comments

ea0f41f

JoshuaLampert requested changes Feb 6, 2025

View reviewed changes

test/test_parabolic_3d.jl Outdated Show resolved Hide resolved

test/test_parabolic_3d.jl Outdated Show resolved Hide resolved

jlchan and others added 2 commits February 6, 2025 13:43

Update test/test_parabolic_3d.jl

1d320f9

Co-authored-by: Joshua Lampert <[email protected]>

Update test/test_parabolic_3d.jl

a572bcb

Co-authored-by: Joshua Lampert <[email protected]>

JoshuaLampert previously approved these changes Feb 6, 2025

View reviewed changes

JoshuaLampert requested a review from ranocha February 6, 2025 19:51

This was referenced Feb 7, 2025

Use Adapt.jl to change storage and element type #2212

Open

Release v0.10 #2105

Closed

ranocha reviewed Feb 7, 2025

View reviewed changes

add news and bump dev version

536f6a7

ranocha dismissed JoshuaLampert’s stale review via 536f6a7 February 7, 2025 07:41

ranocha added the breaking label Feb 7, 2025

ranocha approved these changes Feb 7, 2025

View reviewed changes

ranocha merged commit 9f2a5be into main Feb 7, 2025
31 of 34 checks passed

ranocha deleted the jc/wrap_VectorOfArray branch February 7, 2025 07:45

JoshuaLampert mentioned this pull request Feb 10, 2025

Switch DGMulti to Matrix{<:SVector} solution storage #1952

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use `VectorOfArray` in `wrap_array` for `DGMulti` solvers #2150

Use `VectorOfArray` in `wrap_array` for `DGMulti` solvers #2150

jlchan commented Nov 7, 2024 •

edited

Loading

github-actions bot commented Nov 7, 2024

codecov bot commented Nov 7, 2024 •

edited

Loading

jlchan commented Nov 7, 2024 •

edited

Loading

DanielDoehring commented Nov 8, 2024

jlchan commented Nov 8, 2024

DanielDoehring commented Nov 9, 2024

jlchan commented Nov 10, 2024 •

edited

Loading

huiyuxie commented Nov 11, 2024

huiyuxie commented Nov 11, 2024

jlchan commented Nov 11, 2024

huiyuxie commented Nov 11, 2024

JoshuaLampert left a comment

jlchan commented Feb 6, 2025 •

edited

Loading

JoshuaLampert commented Feb 6, 2025

ranocha Feb 7, 2025

Use VectorOfArray in wrap_array for DGMulti solvers #2150

Use VectorOfArray in wrap_array for DGMulti solvers #2150

Conversation

jlchan commented Nov 7, 2024 • edited Loading

github-actions bot commented Nov 7, 2024

Review checklist

Purpose and scope

Code quality

Documentation

Testing

Performance

Verification

codecov bot commented Nov 7, 2024 • edited Loading

Codecov Report

jlchan commented Nov 7, 2024 • edited Loading

DanielDoehring commented Nov 8, 2024

jlchan commented Nov 8, 2024

DanielDoehring commented Nov 9, 2024

jlchan commented Nov 10, 2024 • edited Loading

huiyuxie commented Nov 11, 2024

huiyuxie commented Nov 11, 2024

jlchan commented Nov 11, 2024

huiyuxie commented Nov 11, 2024

JoshuaLampert left a comment

Choose a reason for hiding this comment

jlchan commented Feb 6, 2025 • edited Loading

JoshuaLampert commented Feb 6, 2025

ranocha Feb 7, 2025

Choose a reason for hiding this comment

Use `VectorOfArray` in `wrap_array` for `DGMulti` solvers #2150

Use `VectorOfArray` in `wrap_array` for `DGMulti` solvers #2150

jlchan commented Nov 7, 2024 •

edited

Loading

codecov bot commented Nov 7, 2024 •

edited

Loading

jlchan commented Nov 7, 2024 •

edited

Loading

jlchan commented Nov 10, 2024 •

edited

Loading

jlchan commented Feb 6, 2025 •

edited

Loading