DM-15914: Use obs_metadata for all header translations and unit extraction #114
Conversation
timj
force-pushed
the
tickets/DM-15914
branch
2 times, most recently
from
d1d8816
to
9c5c57f
Compare
timj
changed the title
| argDict = dict() | ||
|
|
||
| obsInfo = ObservationInfo(md, translator_class=self.metadataTranslator) | ||
| print(obsInfo.__dict__) |
|
|
||
| # Strip all the cards out that were used | ||
| for c in obsInfo.cards_used: | ||
| del md[c] |
There was a problem hiding this comment.
Could/should this operation be an ObservationInfo method?
There was a problem hiding this comment.
It actually is in ObservationInfo, except the method there deliberately returns a copy of the supplied metadata stripped of headers rather than stripping in place. It seemed a bit naughty to modify in place.
| from astropy.utils import iers | ||
|
|
||
| # This is an unofficial ERFA interface | ||
| import astropy._erfa as erfa |
There was a problem hiding this comment.
This seems slightly worrying; mind augmenting the comment with a bit more on either why we shouldn't worry or why we have no choice?
There was a problem hiding this comment.
Yes, and no. If Astropy ever drops erfa then that will be a problem. On the other hand, the erfa routine in question is pretty simple to reproduce, and there is a python/erfa wrapper extant that we could use.
| ut1time = middle | ||
|
|
||
| era = erfa.era00(ut1time.jd1, ut1time.jd2) | ||
| argDict["era"] = era * radians |
There was a problem hiding this comment.
This seems like the kind of calculation I'd have expected to happen inside obs_metadata. Are you just trying to be compatible with VisitInfo's definition of a quantity, while disliking that definition enough to not want it in ObservationInfo?
There was a problem hiding this comment.
Earth rotation angle is entirely a property of the time of observation. It's not something that I felt should exist in ObservationInfo and it's not something that is really needed if you are using Astropy coordinates everywhere. afw VisitInfo needs it because it's not got access to astropy. This calculation exists here entirely to be compatible with afw.
| self.log.warn("argDict[{}] is None; stripping".format(key, argDict[key])) | ||
| del argDict[key] | ||
|
|
||
| print("CCCCCCCCCCCCCVC VISIT INFO CCCCCCCCCCCC") |
| return None | ||
|
|
||
| def to_detector_exposure_id(self): | ||
| return self.to_exposure() |
There was a problem hiding this comment.
Docstrings or comments on this helper class would be helpful (doesn't need anything huge or formal, just an explanation of the role this plays in the tests).
timj
force-pushed
the
tickets/DM-15914
branch
2 times, most recently
from
16b84f2
to
8934873
Compare
timj
force-pushed
the
tickets/DM-15914
branch
3 times, most recently
from
6c7cff2
to
1e864c9
Compare
…traction Many methods no longer abstract because astro_metadata_translator is handling the conversions. VisitInfo is now derived directly from an ObservationInfo. Leave the existing VisitInfo classes to allow for staged migration of obs packages to the new approach.
timj
force-pushed
the
tickets/DM-15914
branch
from
1e864c9
to
b65a9e8
Compare
Many methods no longer abstract because obs_metadata is handling the conversions. VisitInfo is now derived directly from an ObservationInfo.
Leave the existing VisitInfo classes to allow for staged migration of obs packages to the new approach.