Fix CylindricalBinaryCompactObject bugs in grid anchors and time dependent maps#7225
Conversation
This fixes a bug where control_system::ControlErrors::Expansion::operator() was getting the incorrect x, y, z coordinates of the grid positions via Parallel::get<domain::Tags::ObjectCenter<domain::ObjectLabel::A>> (and same for object B).
| // rotation + translation map from the grid to inertial frame. No maps to | ||
| // the distorted frame | ||
| grid_to_inertial_block_maps[first_outer_shell_block] = | ||
| grid_to_inertial_block_maps[74] = |
There was a problem hiding this comment.
It looks like later on line 987 this is still accessed as grid_to_inertial_block_maps[first_outer_shell_block]. Is that still right?
Also, if the first outer shell block is always 74 we shouldn't have a variable first_outer_shell_block containing something else, and if it isn't always 74 then the comment on this line is no longer correct. One of these should be fixed, but I'm not sure which one.
There was a problem hiding this comment.
74 is still correct in this case because when both inner spheres are included, it's 74, and we currently have it that if time dependent options are used, both inner spheres and the outer one must be used. However, I think this is pretty fragile and less maintainable. There is a reason we have first_outer_shell_block as a variable, and it's because it can change. I'm going to revert this 74->first_outer_shell_block in a fixup because that's definitely better, especially given that we'll be adding support for spherical shells for the inner spheres, which will shift these block numbers around.
There was a problem hiding this comment.
Yeah, all these magic numbers floating around aren't great, but your change makes sense now. Looks good.
Blocks in outer sphere (blocks >= 74) should have the transition expansion, but it had the rigid expansion.
Removes unused variable and adds missing consts
macedo22
force-pushed
the
cbco_fix_grid_anchors_and_time_dep_maps
branch
from
7334d18 to
2cc7476
Compare
|
@wthrowe Directly squashed in the |
2 participants
Proposed changes
This fixes a couple bugs in
CylindricalBinaryCompactObjectinvolving the grid achors orientation and time dependent maps in the outer sphere region. Details are in the commit messagesUpgrade instructions
Code review checklist
make docto generate the documentation locally intoBUILD_DIR/docs/html.Then open
index.html.code review guide.
bugfixornew featureif appropriate."Co-Authored-By: Claude Sonnet 4.6 noreply@anthropic.com",
"Co-Authored-by: Codex noreply@openai.com", or
"Co-Authored-By: GitHub Copilot CLI noreply@microsoft.com"
as the last line of the commit, depending on the agent.
Further comments