Skip to content

Conversation

ebellm
Copy link
Contributor

@ebellm ebellm commented Mar 21, 2022

No description provided.

This reverts commit a75ab0b, reversing
changes made to e14b609.
@ebellm ebellm force-pushed the tickets/DM-34132 branch from 45c96b4 to 9fed6ea Compare March 21, 2022 18:43
@ebellm ebellm requested a review from kfindeisen March 21, 2022 18:59
@ebellm ebellm changed the title Tickets/dm 34132 DM-34132: Use shared visit.py with pure python types Mar 21, 2022
Copy link
Member

@kfindeisen kfindeisen left a comment

Choose a reason for hiding this comment

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

There's one visible bug (an out-of-date call to Visit()). Can you test upload.py in the cloud, or should I try to do it?

Comment on lines +13 to +17
ra: float
dec: float
rot: float
Copy link
Member

Choose a reason for hiding this comment

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

I suggest adding a comment that these are all in degrees.

import lsst.geom


@dataclass(frozen=True)
Copy link
Member

Choose a reason for hiding this comment

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

Please add a comment warning that Visit's members should be builtins, and must be both hashable and JSON-persistable. Otherwise we'll just make the same mistake again.

@ebellm ebellm force-pushed the tickets/DM-34132 branch from c99c9cb to a060fb6 Compare March 21, 2022 19:30
@ebellm ebellm force-pushed the tickets/DM-34132 branch from a060fb6 to 9171e72 Compare March 21, 2022 19:33
@ebellm
Copy link
Contributor Author

ebellm commented Mar 21, 2022

@kfindeisen, can you take another look? I confirmed that upload.py is working, with HSC as well as a simulated instrument.

@ebellm ebellm merged commit c3cb917 into main Mar 21, 2022
@ebellm ebellm deleted the tickets/DM-34132 branch March 21, 2022 19:38
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.

2 participants