Repository navigation
Conversation
eteq
changed the title
|
looks good to me @eteq |
mhvk
left a comment
mhvk
left a comment
There was a problem hiding this comment.
Two comments, which lead to a larger one: I think it is a big mistake to rush this - I feel quite strongly that __eq__ should, like ndarray, return a boolean array, and we need to think much harder about what we do with differentials. I think for now we should just move this logic to is_equivalent_frame, where it is much clearer what we actually want to happen. And then it is a proper bug fix, not the introduction of a new feature.
|
|
||
| def __eq__(self, other): | ||
| if not isinstance(other, BaseRepresentationOrDifferential): | ||
| return False |
There was a problem hiding this comment.
This is wrong: should return NotImplemented (CPython will convert to False).
| if isinstance(other, self.__class__): | ||
| for comp in self.components: | ||
| if np.any(getattr(self, comp) != getattr(other, comp)): | ||
| return False |
There was a problem hiding this comment.
Why are we not returning the array here?
There was a problem hiding this comment.
Ah, very good point. I had originally been thinking that there's no natural way to do it due to components being ambiguous, but now that you bring it up you're absolutely right that it's just an np.all(...) outside the loop.
|
Yes, let's postpone this one to 3.2. The reason to like arrays is that I also see representations as a vector-array class, and for arrays the default is to return array of bool. Plus, it is easy to check equality of all for an array, but not easy to get out equality for individual items if they have already been combined. That said, numpy does break python expectations with this, which is a pain. |
|
Yes, I agree with @mhvk! |
|
@StuartLittlefair @mhvk @adrn - well 3.2 freeze is coming close again... I implemented the suggestions @mhvk made on the version from 6 months back and did a rebase. So the two items to discuss are:
We could consider doing a "bigger" break (both in the differentials and the representations) in 4.0 perhaps? |
|
On (1), I think you mean that changing it to returning an array will be an API breaking change if we merge this, right? I'm still not at all comfortable with checking equality of all elements: in many ways representations can be thought of as structured arrays or tables, and for those too you would expect that comparison would be by row rather than for all. I'm wondering if we aren't better of to define |
|
👍 to the idea of having |
|
Gotcha, I like that suggestion too @mhvk. No time to do that now as it would be a pretty major change to this PR, so I'm re-milestoning this to 4.0. |
|
(Note that I'm more than happy for anyone else to take this on though...) |
|
Hi humans 👋 - this pull request hasn't had any new commits for approximately 5 months. I plan to close this in a month if the pull request doesn't have any new commits by then. In lieu of a stalled pull request, please consider closing this and open an issue instead if a reminder is needed to revisit in the future. Maintainers may also choose to add If this PR still needs to be reviewed, as an author, you can rebase it to reset the clock. If you believe I commented on this pull request incorrectly, please report this here. |
|
I'm going to close this pull request as per my previous message. If you think what is being added/fixed here is still important, please remember to open an issue to keep track of it. Thanks! If this is the first time I am commenting on this issue, or if you believe I closed this issue incorrectly, please report this here. |
This fixes #8138 by adding an equality comparison for representation objects. Some implementation notes:
Note that this is high priority as it is currently a blocker for release of 3.1 because it fixes #8138 (somewhat counter-intuitively by creating a new feature
cc @adrn @bsipocz @astrofrog (the latter mostly because it probably needs a 👍 since we're post feature-freeze... although debatable since it's both a new feature and a bug fix?)