-
Notifications
You must be signed in to change notification settings - Fork 192
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 Skew map to BCO #6447
base: develop
Are you sure you want to change the base?
Add Skew map to BCO #6447
Conversation
Rebased and no longer dependent |
@@ -469,8 +488,14 @@ TimeDependentMapOptions<IsCylindrical>::distorted_to_inertial_map( | |||
: RotScaleTrans{}; | |||
|
|||
if (block_has_shape_map) { | |||
if (rot_scale_trans_map_.has_value()) { | |||
if (rot_scale_trans_map_.has_value() and | |||
(use_rigid_map and skew_map_.has_value())) { |
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.
If use_rigid_map=false
but both rot_scale_trans
and skew
maps are given, this will ignore the skew map. Is that what you want? Could you make this clearer, maybe with a comment or by throwing errors or something
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 this is correct. I'll add a comment
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.
So when use_rigid_map=false
the skew map can't be applied? Better throw an error then if the skew map was requested.
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 "can't" be applied, but "won't" be applied. If there is a skew map, but the block we are in is in the outer shell, then use_rigid_map=false
and skew_map_.has_value()=true
, yet we don't want to apply the skew map
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 that clarifies it, thanks 👍
I think in this code branch and also in the block_has_shape_map
branch in grid_to_distorted_map
below we're always within the inner cubes, so use_rigid_map
should be always true
. Maybe assert that instead of the sentinel.
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.
Added some errors
Proposed changes
Adds the Skew map to the BinaryCompactObject domain. Sets it to None in all input files for now until a control system is written for it.
Part of #4603.
Upgrade instructions
When using the BinaryCompactObject domain creator, you must add the following option to the time depndent options:
It has the same capabilities to be
None
or from a volume file as the other time dependent map options.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
Depends on and includes #6446.