Towards partitioned RHS's (1D) #1967
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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 ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1967 +/- ##
=======================================
Coverage 96.36% 96.36%
=======================================
Files 480 480
Lines 38028 38028
=======================================
Hits 36645 36645
Misses 1383 1383
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
From a first look, I like the idea! An small variation could be to have a modified function without default argument and create new functions with the old API that call it with Another thought is that to me, |
|
In general, I think this would be a good topic to bring up at a Trixi.jl meeting for discussion (possibly with a heads up in Slack such that people can think about it beforehand) |
True, but this adds in principle the overhead of calling another function, right? But should be explored in a benchmarking run.
No, not yet.
Yeah that sounds reasonable 👍 |
Yes and yes. In the end, it is likely that it doesn't make a difference performance-wise. |
|
Sounds reasonable. Could you please run some benchmarks? |
|
Currently, we do not benchmark any 1D simulation: Trixi.jl/benchmark/benchmarks.jl Lines 13 to 39 in 961f2e7 I could add some 1D elixirs locally or we extend the benchmarks by some representative elixirs for 1D. Otherwise, I could extend these changes to 2D to get benchmark results for this. |
|
I think it would be nice if you could add some representative 1D elixirs to the benchmarks in a new PR |
This was referenced Jul 8, 2024
|
Okay, so here are some benchmark results (Only one thread, two threads is basically no output). Benchmark Report for /home/daniel/git/Trixi.jlJob Properties
ResultsA ratio greater than
Benchmark Group ListHere's a list of all the benchmark groups executed by this job:
Julia versioninfoTargetBaseline |
|
Results of main vs main: 1 Thread: Benchmark Report for /home/daniel/git/Trixi.jlJob Properties
ResultsA ratio greater than
Benchmark Group ListHere's a list of all the benchmark groups executed by this job:
Julia versioninfoTargetBaseline2 Threads: Benchmark Report for /home/daniel/git/Trixi.jlJob Properties
ResultsA ratio greater than
Benchmark Group ListHere's a list of all the benchmark groups executed by this job:
Julia versioninfoTargetBaseline |
|
@vchuravy can you maybe take a look at this (if you have the time)? I really lack the experience to be able to judge these results :/ |
…l into PartitionedRHS_1D
|
I took a look at the differences of the lowered form of the function for the Here is the lowered code of the new version: julia> @code_lowered Trixi.pure_and_blended_element_ids!(element_ids_dg, element_ids_dgfv, alpha, solver, cache, eachelement(solver, cache))
CodeInfo(
1 ─ Trixi.empty!(element_ids_dg)
│ Trixi.empty!(element_ids_dgfv)
│ %3 = element_indices
│ @_8 = Base.iterate(%3)
│ %5 = @_8 === nothing
│ %6 = Base.not_int(%5)
└── goto #7 if not %6
2 ┄ %8 = @_8
│ element = Core.getfield(%8, 1)
│ %10 = Core.getfield(%8, 2)
│ %11 = Base.getindex(alpha, element)
│ %12 = (:atol,)
│ %13 = Core.apply_type(Core.NamedTuple, %12)
│ %14 = Core.tuple(1.0e-12)
│ %15 = (%13)(%14)
│ dg_only = Core.kwcall(%15, Trixi.isapprox, %11, 0)
└── goto #4 if not dg_only
3 ─ Trixi.push!(element_ids_dg, element)
└── goto #5
4 ─ Trixi.push!(element_ids_dgfv, element)
5 ┄ @_8 = Base.iterate(%3, %10)
│ %22 = @_8 === nothing
│ %23 = Base.not_int(%22)
└── goto #7 if not %23
6 ─ goto #2
7 ┄ return Trixi.nothing
)For the currently implemented one: julia> @code_lowered Trixi.pure_and_blended_element_ids!(element_ids_dg, element_ids_dgfv, alpha, solver, cache)
CodeInfo(
1 ─ Trixi.empty!(element_ids_dg)
│ Trixi.empty!(element_ids_dgfv)
│ %3 = Trixi.eachelement(dg, cache)
│ @_7 = Base.iterate(%3)
│ %5 = @_7 === nothing
│ %6 = Base.not_int(%5)
└── goto #7 if not %6
2 ┄ %8 = @_7
│ element = Core.getfield(%8, 1)
│ %10 = Core.getfield(%8, 2)
│ %11 = Base.getindex(alpha, element)
│ %12 = (:atol,)
│ %13 = Core.apply_type(Core.NamedTuple, %12)
│ %14 = Core.tuple(1.0e-12)
│ %15 = (%13)(%14)
│ dg_only = Core.kwcall(%15, Trixi.isapprox, %11, 0)
└── goto #4 if not dg_only
3 ─ Trixi.push!(element_ids_dg, element)
└── goto #5
4 ─ Trixi.push!(element_ids_dgfv, element)
5 ┄ @_7 = Base.iterate(%3, %10)
│ %22 = @_7 === nothing
│ %23 = Base.not_int(%22)
└── goto #7 if not %23
6 ─ goto #2
7 ┄ return Trixi.nothing
)The
|
|
Should be fine, I hope... Maybe you can run one or two benchmarks manually to check. |
…l into PartitionedRHS_1D
|
Here are benchmarks for the 1D tests only. Looks as expected. Benchmark Report for /storage/home/daniel/git/Trixi.jlJob Properties
ResultsA ratio greater than
Benchmark Group ListHere's a list of all the benchmark groups executed by this job:
Julia versioninfoTargetBaselineBenchmark Report for /storage/home/daniel/git/Trixi.jlJob Properties
ResultsA ratio greater than
Benchmark Group ListHere's a list of all the benchmark groups executed by this job:
Julia versioninfoTargetBaseline |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This draft is one way how we could realize partitioned RHS's with relatively little changes to the existing code.
Opinions on this @sloede ?
Related to #21