Repository navigation
Fix repr is equiv simpler - #8218
Conversation
Codecov Report
@@ Coverage Diff @@
## master #8218 +/- ##
=========================================
Coverage ? 86.91%
=========================================
Files ? 383
Lines ? 57885
Branches ? 1056
=========================================
Hits ? 50309
Misses ? 6962
Partials ? 614
Continue to review full report at Codecov.
|
| return True | ||
| elif isinstance(right_fattr, r.BaseRepresentationOrDifferential): | ||
| # both are representations. Note that this ignores any | ||
| # stored differentials |
There was a problem hiding this comment.
What about raising a warning if either right_fattr or left_fattr has differentials present?
There was a problem hiding this comment.
I'd go further and just return False - right now, we're even stricter and we can relax in steps.
There was a problem hiding this comment.
In the version I just pushed up I'm doing both of these things now.
I think it's important to have the warning as @StuartLittlefair suggested because otherwise it's very difficult to understand what's happening if by chance anyone gets bit by this behavior. I don't think it's relevant for any frame I've ever seen, but I think it's safer just in case because it's such a subtle thing.
Relatedly: re: @mhvk's comment, the one danger here is that it's possible we accidentally introduced a strictness here - the core problem from #8138 is that previously a lot of this could be ignored because we didn't copy representation frame attributes but rather just transferred over the references. That was not really intentional, but it masked the issue this PR is fixing. So if I'm wrong with the above and someone really is using a frame attribute with a representation that is supposed to have a differential, this might now bite them where before it was fine because it was passed by reference. I think this could be in the dictionary as a "corner case", though, so I'm not too worried. The only possibility is Sunpy... So we might want @Cadair to give a quick 👍 that this isn't used somewhere there?
|
this now needs a rebase |
1e47314 to
5317d64
Compare
|
Assuming the tests pass I think this is all set (i.e., I think I've addressed the discussion items) as long as @Cadair doesn't see the change discussed in #8218 (comment) doesn't break Sunpy. It does need someone to approve it though (@StuartLittlefair @mhvk @adrn?) |
|
@Cadair - could you please double check it in sunpy? |
StuartLittlefair
left a comment
StuartLittlefair
left a comment
There was a problem hiding this comment.
Happy to approve this, assuming @Cadair doesn’t find it breaks SunPy
mhvk
left a comment
mhvk
left a comment
There was a problem hiding this comment.
Yes, looks all fine. Good to have the warning that the behaviour with differentials may change.
|
Thank you all! |
This is an alternative fix for #8138 vs #8205, as suggested by @mhvk in #8205 (review). Instead of fully defining equality for representations, it does it more narrowly only in the context of
is_equivalent_frame.Note that this should not be thought of as a full replacement for #8205. Rather, this is a bugfix to address #8138 without adding the new feature #8205 adds (a concept of representation equality). So if we opt for this, we should keep #8205 open but re-milestone it for 3.2 (and discuss there whether we really want it, since @mhvk raises some legitimate concerns).