-
Notifications
You must be signed in to change notification settings - Fork 104
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
Remove ndims
where not necessary
#2081
Remove ndims
where not necessary
#2081
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. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2081 +/- ##
==========================================
+ Coverage 96.32% 96.34% +0.02%
==========================================
Files 470 470
Lines 37485 37494 +9
==========================================
+ Hits 36106 36121 +15
+ Misses 1379 1373 -6
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
resize!(u_ode, | ||
nvariables(equations) * nnodes(dg)^ndims(mesh) * nelements(dg, cache)) | ||
nvariables(equations) * nnodes(dg)^2 * nelements(dg, cache)) | ||
u = wrap_array(u_ode, mesh, equations, dg, cache) |
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 think parts like these become much harder to understand with this PR. What do you think, @sloede?
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.
Yes, I have to agree with Hendrik.
The advantage of explicitly adding ndims(mesh)
is that it makes the factor self-explanatory. Also in the other parts of the code, one avoids having to explain why a factor is what it is and how it was computed, or even how to change it when switching to another dimension.
I see the argument for more "elegant" code (avoiding the impression of having a varying dimension). However, I've seen too many codes where "magic" constants appear out of nowhere, and everyone reading it after 3 years has to re-understand what the intent/thought process of the original author was.
IMHO, the current version is concise and self-explanatory in terms of what is being done, thus I'd prefer to keep it as it is.
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.
To throw in my two cents. I want to keep the ndims
calls in the code. I think it is much more informative to have terms like 2^ndims(mesh) - 1
rather than values like 3
and 7
. This is coming from the perspective of someone who recently became very familiar with the AMR implementation as I debugged and tracked down the loss of conservation. I think the current code is perfectly understandable.
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.
Ok, I will close this PR then. Opinions about the status of the issue?
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 would close it in light of the comments above
@@ -330,7 +330,7 @@ function coarsen!(u_ode::AbstractVector, adaptor, | |||
# If an element is to be removed, sanity check if the following elements | |||
# are also marked - otherwise there would be an error in the way the | |||
# cells/elements are sorted | |||
@assert all(to_be_removed[old_element_id:(old_element_id + 2^ndims(mesh) - 1)]) "bad cell/element order" | |||
@assert all(to_be_removed[old_element_id:(old_element_id + 2^2 - 1)]) "bad cell/element order" |
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.
One could move skip = 3
already up here
@@ -250,7 +250,7 @@ function coarsen!(u_ode::AbstractVector, adaptor, | |||
# If an element is to be removed, sanity check if the following elements | |||
# are also marked - otherwise there would be an error in the way the | |||
# cells/elements are sorted | |||
@assert all(to_be_removed[old_element_id:(old_element_id + 2^ndims(mesh) - 1)]) "bad cell/element order" | |||
@assert all(to_be_removed[old_element_id:(old_element_id + 2^3 - 1)]) "bad cell/element order" |
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.
One could move skip = 7
already up here
@@ -187,13 +187,15 @@ function coarsen!(u_ode::AbstractVector, adaptor, mesh::TreeMesh{1}, | |||
# If an element is to be removed, sanity check if the following elements | |||
# are also marked - otherwise there would be an error in the way the | |||
# cells/elements are sorted | |||
@assert all(to_be_removed[old_element_id:(old_element_id + 2^ndims(mesh) - 1)]) "bad cell/element order" | |||
@assert all(to_be_removed[old_element_id:(old_element_id + 2 - 1)]) "bad cell/element order" |
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.
One could move skip = 1
already up here
See discussion |
Closes #2080