Skip to content
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

DM-27477: Add JSON serialization for ObservationInfo #48

Merged
merged 7 commits into from Feb 17, 2021
Merged

Conversation

timj
Copy link
Member

@timj timj commented Feb 15, 2021

No description provided.

Now explicitly check to ensure that the types match.
This required addition of new helper functions in the properties
definition. These are used to convert Astropy objects to simple
form and to convert that simple form back to Astropy.

Adding this made the __eq__ method much more robust since now
the simplified forms are compared directly rather than
comparing stringified forms.
@timj timj force-pushed the tickets/DM-27477 branch 3 times, most recently from 9fc507b to 5c01287 Compare February 16, 2021 16:44
@timj timj requested a review from erykoff February 16, 2021 17:19
Copy link

@erykoff erykoff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks fine. I was confused about the PROPERTIES and _PROPERTIES but I think that's mostly because github only shows the diff by default.

@@ -108,6 +108,9 @@ def __iter__(self):
def __eq__(self, other):
"""Compares equal if all the members are equal in the same order.
"""
if not isinstance(other, ObservationGroup):
return NotImplemented
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After looking into this, I have now learned about NotImplemented as a valid return for __eq__.

If both of them have nan then we treat that as equal.
@timj timj merged commit 05ce667 into master Feb 17, 2021
@timj timj deleted the tickets/DM-27477 branch February 17, 2021 15:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants