-
Notifications
You must be signed in to change notification settings - Fork 1
Fix concatenate tests. #18
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
Conversation
|
@lbdreyer could you cast you eye over these changes to make sure you're happy with them. If you are then I'll merge (since you don't have merge rights on my repo 😉), otherwise comment on issue points and we'll discuss |
| # Setting the cube.data clears the cube.dtype and cube.fill_value. | ||
| # We want to maintain the fill_value for testing purposes, even | ||
| # though we have lost the lazy data in order to pin down the array | ||
| # data order to overcome testing on different architectures. |
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.
Should we be putting this comment everywhere we have done
fill_value = cube.fill_value
cube.data = ...
cube.fill_value = fill_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.
I've tried to leave some clue's (for my future self) but to be honest if we have a step change in the "persisting fill_value across a data setter operation" then we can always find the usage of fill_value in the code base.
I just added a bit more of context for concatenate because it's testing is a bit weird and, at the time, I thought an exception to the norm.
| cube.data = ma.array(data, mask=mask, fill_value=fill_value) | ||
| cube.data = ma.array(data, mask=mask) | ||
| else: | ||
| # cube.data = np.copy(cube.data, order=order) |
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.
Do you know why these were commented out in the first place?
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 was changed by @pp-mo 3 years ago ... I don't see the need to keep them
|
👍 Happy with the changes 👍 |
Ping @lbdreyer