Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
ENH: support alpha arrays in collections #6268
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
ENH: support alpha arrays in collections #6268
Changes from all commits
821617b
c9d0741
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Stall a couple of code paths not tested? (L625 and 632)
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 not want to check this before calling the setter?
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.
No, I think it is better to have the more basic argument-checking from the setter before checking the number of dimensions.
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.
Wouldn't that leave
self.alpha
in an inconsistent state?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.
Why would that matter? An exception is being raised either way. Look at
_ImageBase.set_data
. All of the validation is being done after the attribute assignment:It looks like the
set_data
code would be slightly more efficient and readable if the attribute assignment were done at the end, but that's not the issue here. Forset_alpha
, both efficiency and logical order of validation checks are better with the ndim check afterset_alpha_for_array
.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.
To resolve this, can we easily write a test that shows how it works?
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 see anything to resolve. I have already included a test for this case, raising an exception if the wrong number of dimensions is supplied for an image.
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.
FWIW, if we already have cases of inconsistent state on invalid input, then I'm slightly less concerned about adding one more.
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 is impossible to do all validation before setting attributes, given independent setters, because valid alpha depends on data, and vice-versa. That's why one of the tests I added calls
update_scalarmappable
; that's where the validation has to be, because that is where we have both alpha and data and can cross-check them.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 at somepoint someone will put some masked or NaN into alpha. What will happen, and what do you recommend?
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.
Fail on nan; ignore mask. I don't think it is worthwhile for us to try to propagate nans and masked arrays from the alpha input.