DM-44605: Prompt processing can do ISR for LATISS calibrations #181
Conversation
|
@kfindeisen do you mind taking a look of some additional commits for this ticket? |
| _log.warning("Got non-ICRS coordinates %s in Visit %s.", | ||
| repr(self.visit.coordinateSystem), self.visit) |
There was a problem hiding this comment.
| _log.warning("Got non-ICRS coordinates %s in Visit %s.", | |
| repr(self.visit.coordinateSystem), self.visit) | |
| _log.warning("Got non-ICRS coordinates %r in Visit %s.", | |
| self.visit.coordinateSystem, self.visit) |
There was a problem hiding this comment.
Won't this still break when trying to preload spatial datasets (refcats and templates)?
There was a problem hiding this comment.
The nextVisit of those calibration frames have [ra,dec]=[0,0], so they don't "break" but just waste time copying non-useful data. But it's true that we may not rely on them always being [0,0] as the default; we might tell summit to do so assuming we can have our own special "prompt-processing-test" observing script.
There was a problem hiding this comment.
I'm confused. What is the resolution of this issue?
There was a problem hiding this comment.
(Sorry. I did not remember hitting "Resolve conversation" here and must have been a mistake) (Or did GitHub think it was resolved because I committed the suggested change?)
There was a problem hiding this comment.
Coming back to this after a long time...
A bad value for dec indeed would break prep_butler, but any numeric values for ra and cameraAngle would be fine; any processed results after ISR would be suspicious of course. I changed it so it still raises if values are obviously bad, otherwise just logs a message and continue.
For the default case of [ra,dec]=[0,0], I think wasting time downloading potentially useless refcats is not a big deal.
What do you think?
There was a problem hiding this comment.
I'd prefer to make loading of spatial datasets be conditional on having spatial coordinates in the first place, just because it's less error-prone than something that just happens to work. But I agree that run time is not a problem.
There was a problem hiding this comment.
I hesitated on making these visits handled any differently and found the happenstance convenient, but you have a good point. It's indeed more robust to not use those coordinates at all. The last commit has been rewritten, and now the pointless loading will be skipped for non-on-sky visits.
hsinfang
force-pushed
the
tickets/DM-44605
branch
from
8169bb9
to
f7255a2
Compare
hsinfang
force-pushed
the
tickets/DM-44605
branch
from
f7255a2
to
015d6dd
Compare
Instead of constraining visit_system to 0 in the data query, only define the one-to-one (visitSystem=0) visits in the repo. This preserves the bahavior and makes it also work with non-on-sky exposures that cannot take visit constraint in the data query.
ts_idl package is being deprecated and the enumerations are moved to ts_xml package.
hsinfang
force-pushed
the
tickets/DM-44605
branch
7 times, most recently
from
38058c0
to
06a13c7
Compare
| self._skip_spatial_preload = True | ||
| _log.error("Only ICRS coordinates are fully supported. " | ||
| f"Got {self.visit.coordinateSystem!r} instead in {self.visit}. " | ||
| "Spatial datasets won't be loaded.") |
There was a problem hiding this comment.
Should this be a warning, given that we are plowing ahead anyway?
There was a problem hiding this comment.
I slightly prefer this to be an error for now, so (1) it does not drown in so many warnings from the pipelines, and (2) in a way it is an error as the system isn't really designed to process those visits and something may go wrong later. I feel that this is similar to the level of finding no templates for some visits but we continue with the next best thing.
Instead of raising an error, allow non-on-sky visits but log an error and do not use those coordinates and camera angles.
hsinfang
force-pushed
the
tickets/DM-44605
branch
from
06a13c7
to
e706255
Compare
hsinfang
changed the title
No description provided.