-
Notifications
You must be signed in to change notification settings - Fork 20
DM-50987: Add sattle to pipelines #1141
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
Conversation
b3e1dfb to
96a3009
Compare
kfindeisen
left a comment
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.
The most glaring issue is the code duplication with lsst/ip_diffim#418, but I also have one big-picture question: what, exactly, is this code for? It doesn't seem to use the Sattle results for anything. Can it all just be removed?
|
@kfindeisen The big picture is that sattle needs the exact exposure time and the boresight ra & dec to populate a cache that will later be used in ip_diffim.detectAndMeasure. That cache takes a small amount of time to populate, so we want to call it early in the pipeline execution process so we're not waiting on it later during detectAndMeasure. This call (to (We have taken steps on the service side to make the service idempotent and able to ignore 189 duplicate requests.) |
I think a comment to that effect would be helpful (though, again, factoring the code might make that unnecessary if the new code is self-descriptive enough). |
kfindeisen
left a comment
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.
Looks good. As before, many of my comments from lsst/ip_diffim#418 apply here as well.
No description provided.