-
Notifications
You must be signed in to change notification settings - Fork 189
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
Control center of mass and linear momentum in initial data. #6212
Conversation
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.
Not a complete review, but please don't abbreviate "center of mass" as com
I don't need to approve before the PR is merged
@@ -311,6 +311,13 @@ class BinaryCompactObject : public DomainCreator<3> { | |||
"x-axis)."}; | |||
}; | |||
|
|||
struct ComOffset { |
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.
Please spell out CenterOfMassOffset
@@ -449,7 +456,8 @@ class BinaryCompactObject : public DomainCreator<3> { | |||
|
|||
BinaryCompactObject( | |||
typename ObjectA::type object_A, typename ObjectB::type object_B, | |||
double envelope_radius, double outer_radius, | |||
std::array<double, 2> com_offset, double envelope_radius, |
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.
also spell out center_of_mass_offset
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.
Not yet a complete review, I'll look at the control loop changes tomorrow.
402fb36
to
c445b25
Compare
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.
LGTM please squash 👍 It would help @knelli2 if you split the changes into two commits: one for the domain creator and the other for the control loop.
c5bb000
to
e747476
Compare
@@ -18,9 +18,37 @@ | |||
DEFAULT_RESIDUAL_TOLERANCE = 1.0e-6 | |||
DEFAULT_MAX_ITERATIONS = 30 | |||
|
|||
# Initial data physical parameters that are supported in this control scheme | |||
SupportedParams = Literal[ |
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.
(optional) You can do this to avoid duplicating this list:
SupportedParams = Literal[*FreeDataFromParams.keys()]
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.
It seems like Literal
doesn't take "unpacked arguments", so this would disable the type checking. Since these parameters are specified by users in the input file, I think it would be better to keep it the way 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.
Oh ok it worked on my machine with Py 3.12, but maybe this was a more recent addition to Python. Fine with me.
"Offset in the y and z axes applied to both object A and B in order to " | ||
"control the center of mass."}; |
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.
Can you add a note that this moves the location of the two objects in the grid frame but keeps the Envelope and OuterShell centered on the origin in the grid frame
double x_offset_{}; | ||
double y_offset_{}; | ||
double z_offset_{}; |
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.
Make this a std::array
named center_of_mass_offset_
. With the excision offset that will be added in soon, we'll want to be very explicit about what offset is moving what coordinates.
double y_offset_{}; | ||
double z_offset_{}; |
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.
These are probably ok as is
e747476
to
158528f
Compare
158528f
to
ee7e100
Compare
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! The clang-tidy stuff is unrelated to your changes
Proposed changes
This PR extends the control loop in initial data to adjust the center of mass and ADM linear momentum.
Upgrade instructions
This PR changes
BinaryCompactObject
, which is used in multiple parts of the code. Now, the constructor expects the argumentstd::array<double, 2> center_of_mass_offset
.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