-
Notifications
You must be signed in to change notification settings - Fork 127
Make volume integrals, indicators, etc dimension agnostic #2488
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?
Make volume integrals, indicators, etc dimension agnostic #2488
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. |
…ixi.jl into CleanUpVolumeIntegrals
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2488 +/- ##
==========================================
+ Coverage 96.76% 96.77% +0.01%
==========================================
Files 518 519 +1
Lines 42400 42303 -97
==========================================
- Hits 41027 40938 -89
+ Misses 1373 1365 -8
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.
Thanks for bringing this up. We have to discuss this in a larger group including @sloede and the other @trixi-framework/principal-developers
If we want this (and I am in favor of it), I would propose to put the dimension agnostic functions into dimension-general files, e.g., moving the |
That's an excellent idea! |
…ixi.jl into CleanUpVolumeIntegrals
Following a similar idea, wouldn't it also make sense to put mesh-agnostic functions (at least the ones, which are the same of all mesh types) into mesh-general files like https://github.com/trixi-framework/Trixi.jl/blob/531ee91d0dbe93e8fb8bf9f675b34f7982b9077e/src/solvers/dg.jl. Currently, we have quite some functions, which also allow non- |
I like the idea and I would be fine with adding a file like Lines 830 to 837 in 531ee91
|
Yes, it would be against this code structure, but the question is, which code structure is easier to understand and work with. I was just last week talking to a Trixi.jl newcomer, who wants to contribute to Trixi.jl and was confused about seeing code relevant for the |
As a follow up: Maybe it also makes sense to bundle (in a different PR) identical
This way we can also reduce redundant code and re-do really only the stuff that is actually different, thus giving better code semantics. |
Probably due to historical debt, the volume integrals have been implemented for every dimension. There is currently no reason to do so, as this results only in duplicated code and makes changes such as discussed in #2472 and #2149 unnecessary cumbersome.
Holds also true for some other functions (mostly
create_cache
)