Fix astrometry for LATISS #241
Conversation
python/lsst/obs/lsst/assembly.py
Outdated
| @@ -52,10 +53,19 @@ def attachRawWcsFromBoresight(exposure): | |||
| visitInfo = MakeRawVisitInfoViaObsInfo(logger)(md) | |||
| exposure.getInfo().setVisitInfo(visitInfo) | |||
|
|
|||
| # LATISS (and likely others) need flipping, DC2 etc do not | |||
| obsInfo = ObservationInfo(md) | |||
There was a problem hiding this comment.
There is an implicit double calculation of the ObservationInfo that we don't need. I think if you move this line above the visitInfo line you can then do
visitInfo = MakeRawVisitInfoViaObsInfo.observationInfo2visitInfo(obsInfo, log=logger)
There was a problem hiding this comment.
Yup, for sure, I spotted that but hadn't got around to tidying up late last night and didn't know the right incantation off the top of my head.
python/lsst/obs/lsst/assembly.py
Outdated
| @@ -63,6 +63,9 @@ def attachRawWcsFromBoresight(exposure): | |||
| exposure.setWcs(createInitialSkyWcs(visitInfo, exposure.getDetector(), flipX=flipX)) | |||
| return True | |||
|
|
|||
| if obsInfo.observation_type != "science": | |||
There was a problem hiding this comment.
Are you sure you don't want == "science" here?
There was a problem hiding this comment.
Haha, yes indeed. Sorry, this wasn't actually out for review, I only made the PR to make it easier for Tim to take a glance at the changes.
There was a problem hiding this comment.
In moving this we've lost the ability to put the dataId in the message unless I change to pass it through. It won't need changing anywhere else, so doesn't really change anything, but it would only be for the message - is it worth it? I could default it to None and add it to the message if present as a compromise, but I'm curious about other people's thoughts on elegance/cruft in the code vs. completeness of error messages.
Personally I think that given it will be surrounded by other messages about that dataId it's not really necessary but certainly welcome opinions.
There was a problem hiding this comment.
Consider adding a errmsg=None optional parameter so that the caller can pass in the message with the dataId that can be appended to the generic message.
mfisherlevine
force-pushed
the
tickets/DM-24592
branch
3 times, most recently
from
56915c9
to
293b43e
Compare
python/lsst/obs/lsst/assembly.py
Outdated
| return True | ||
|
|
||
| if obsInfo.observation_type == "science": | ||
| logger.warn("Unable to set WCS from header as RA/Dec/Angle are unavailable%s" % | ||
| ("" if dataIdForErrMsg is None else " for dataId %s" % dataIdForErrMsg)) |
There was a problem hiding this comment.
Don't use % formatting in log calls. That fist % should be ,
There was a problem hiding this comment.
,, + or simply continuation, yes. But I thought the second %s was preferred for log formatting, no?
There was a problem hiding this comment.
I meant the % after the first quote at the end of the line. It should be a , since you are meant to defer the formatting.
mfisherlevine
force-pushed
the
tickets/DM-24592
branch
3 times, most recently
from
c17cf0b
to
1e627cd
Compare
mfisherlevine
force-pushed
the
tickets/DM-24592
branch
3 times, most recently
from
0b3d872
to
49020e4
Compare
mfisherlevine
force-pushed
the
tickets/DM-24592
branch
from
49020e4
to
716b529
Compare
| @@ -0,0 +1 @@ | |||
| ROTPA : 89.739475 | |||
There was a problem hiding this comment.
This is the point where github changed from black text to green "ROTPA", black ":", and blue numbers.
| @@ -0,0 +1 @@ | |||
| ROTPA : 98.214010 | |||
| @@ -0,0 +1 @@ | |||
| ROTPA : -160.418348 | |||
| @@ -0,0 +1 @@ | |||
| ROTPA : -174.461201 | |||
| @@ -0,0 +1 @@ | |||
| ROTPA : -201.012609 | |||
| @@ -0,0 +1 @@ | |||
| ROTPA : -70.517931 | |||
| @@ -0,0 +1 @@ | |||
| ROTPA : -235.947280 | |||
There was a problem hiding this comment.
green. Also where github stops autoloading diffs.
No description provided.