-
Notifications
You must be signed in to change notification settings - Fork 191
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
Add nodegroup support to control system and interpolation framework #6172
Add nodegroup support to control system and interpolation framework #6172
Conversation
const ArrayIndex& /*array_index*/, | ||
tnsr::I<DataVector, Metavariables::volume_dim, | ||
tnsr::I<DataVector, Dim, |
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 had added comments for the previous commits asking to do the opposite of this. Could you give a brief explanation? Was this necessary to compile?
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.
We've generally found "reaching up" into the metavariables to be fairly brittle and tricky. Considering we deduce the dim pretty much everywhere in the code I saw no reason to avoid doing that here. I've also sometimes had issues with GCC not being able to retrieve Metavaraibles::volume_dim
and those were extremely difficult to debug, so I feel like suggesting that as the "default" is not really a good thing.
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 agree in general we shouldn't reach up, but I thought the volume_dim
was an exception since it's so ubiquitous in our metavars. We also do this a lot throughout the code for the dim, so we aren't really consistent. But if it's causing issues with GCC then passing in the Dim explicitly is ok
src/ParallelAlgorithms/Interpolation/Actions/InterpolatorRegisterElement.hpp
Outdated
Show resolved
Hide resolved
54698bc
to
5f4de8c
Compare
@knelli2 I pushed fixups. Thanks for the review :) |
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.
Looks good. You can squash
5f4de8c
to
6670dc5
Compare
Rebased and squashed. Thanks for the reviews! |
6670dc5
to
feb4f37
Compare
Proposed changes
Upgrade instructions
Code review checklist
make doc
to generate the documentation locally intoBUILD_DIR/docs/html
.Then open
index.html
.code review guide.
bugfix
ornew feature
if appropriate.Further comments