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: implement a base comparison operator (__eq__) for NDData #15903
base: main
Are you sure you want to change the base?
Conversation
Thank you for your contribution to Astropy! 🌌 This checklist is meant to remind the package maintainers who will review this pull request of some common things to look for.
|
👋 Thank you for your draft pull request! Do you know that you can use |
71a1416
to
cf500ea
Compare
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.
Equality should ensure that every attribute is "equivalent". Some challenges:
- there isn't an obvious definition of "equivalence" for units – do we allow the user to check for equivalence via strict equivalence (km == km), physical type (AA ~= km), with (our) equivalencies (AA ~= Hz)?
- NDData can have WCS attributes. Is there any functional WCS equivalence check that generalizes well to any APE-14 compliant WCS representation?
- there isn't an equality built-in for
NDUncertainty
classes yet, but I'd argue that if the uncertainties are different, the NDData objects are not equivalent. We could implement that as part of this PR, but that will take some care, since someNDUncertainty
subclasses can be equivalently represented as others. - how would we want this to behave for subclasses? Should we ever let an instance of CCDData == an instance of NDData, when CCDData may also have a
flags
?
Description
This pull request is to address #11648
Actually it's not completely clear to me wether there's actually a consensus on doing this in any way or form:
NDDataBase
class should not define any arithmetic operations, which are impossible to generalize" (though it doesn't specify thatNDData
shouldn't get a__eq__
so the possibility seems open)I'm opening this at an early stage in hopes to get the discussion going, or that the PR be rejected before it consumes too much time.
I'm aware that if a consensus is to be reached, most likely we'll want to implement comparison on more than just
data
,mask
andunit
, but that's also open to discussion: are there any attributes inNDData
that we should not compare in__eq__
?If this is greenlit and more attributes indeed need to be compared, I'll probably need to upgrade the test using hypothesis to improve coverage.
Fixes #11648