-
Notifications
You must be signed in to change notification settings - Fork 51
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
setshape() should not touch the joined dim #1656
base: dev
Are you sure you want to change the base?
Conversation
Do you have a specific use case in mind for this? If there is no specific use case, should we maybe rather just check this and throw an error if the shape is changed in unconventional ways? Also, I think that this logic is more adequately placed in the frontend at |
@@ -2032,7 +2035,18 @@ namespace detail | |||
} | |||
else | |||
{ | |||
var.SetShape(shape); |
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 don't think that the previous shape
should be considered here at all.
This code is executed when the Variable was already part of the previous step. The Variable shape can be something completely new in the new step. If the user means to use a joined dimension again, Dataset::JOINED_DIMENSION
should be specified again; we should not try to outsmart the user by trying to keep the joined dimension from the previous step.
{ | ||
dims.push_back(ext); | ||
auto idx = joined_dim.value(); | ||
dims[idx] = var.Shape()[idx]; |
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.
Undefined behavior if the vector lengths are not the same
Yes, this change was needed when testing joined dimension in warpx. Let me see if I can come up with a test case. |
if a joined dim exists, setshape() should not change it.