Open DanielDoehring opened 3 weeks ago
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.
NEWS.md
with its PR number.Created with :heart: by the Trixi.jl community.
All modified and coverable lines are covered by tests :white_check_mark:
Project coverage is 96.16%. Comparing base (
def208b
) to head (78a7786
).
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
This draft is one way how we could realize partitioned RHS's with relatively little changes to the existing code. Opinions on this @sloede ?
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 eachelement(...)
etc. But I don't know if that's really more readable. Have you checked if your change impacts the performance (since now maybe the compiler is not able to infer as much as before)?
Another thought is that to me, range
strongly implies an ordered subset of an array, e.g., something like 5:10
or 10:2:20
. However, in this case there is no such a restriction - it is just a bunch of indices. So maybe {element,interface,whatever}_indices
would be more apt?
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)
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
eachelement(...)
etc. But I don't know if that's really more readable.
True, but this adds in principle the overhead of calling another function, right? But should be explored in a benchmarking run.
Have you checked if your change impacts the performance (since now maybe the compiler is not able to infer as much as before)?
No, not yet.
Another thought is that to me,
range
strongly implies an ordered subset of an array, e.g., something like5:10
or10:2:20
. However, in this case there is no such a restriction - it is just a bunch of indices. So maybe{element,interface,whatever}_indices
would be more apt?
Yeah that sounds reasonable :+1:
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
eachelement(...)
etc. But I don't know if that's really more readable.True, but this adds in principle the overhead of calling another function, right? But should be explored in a benchmarking run.
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:
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 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 https://github.com/trixi-framework/Trixi.jl/issues/21