DM-16253: Major improvements for DECam and CFHT #4
Conversation
We may change our mind on this.
Each is tried in turn until one is found.
These seem to have different header names for many common headers.
This way we test all the headers even if not all pass.
| import sys | ||
| import traceback | ||
| import argparse | ||
| import yaml |
There was a problem hiding this comment.
Nitpick, but shouldn't these be alphabetical?
| for file in sys.argv[1:]: | ||
| print(f"{file}:") | ||
| md = read_metadata(file) | ||
| re_default = r"\.fit[s]?\b" |
There was a problem hiding this comment.
The entire package is written with underscore naming. We made this decision early on because the translation methods to_datetime_begin mapping to butler gen 3 datetime_begin was a lot more obvious than using toDatetimeBegin. Also we hope to make the package astropy affiliated and thought it best to be more standard with the rest of the community. The public interface (ObservationInfo is very thin).
| Name of the translation method to cache. Not needed if the decorator | ||
| is used around a normal method, but necessary when the decorator is | ||
| being used in a metaclass. | ||
| """ |
| Name of the key to look up in the header. | ||
| header_key : `str` or `list` of `str` | ||
| Name of the key to look up in the header. If a `list` each | ||
| key will be tested in turn. This can deal with header styles |
There was a problem hiding this comment.
It would be more clear to say "will be tested in turn until one matches."
| Is a science observation. | ||
| """ | ||
| if self.to_observation_type() == "science": | ||
| raise KeyError("Header represents science observation and can not default") |
There was a problem hiding this comment.
As a user, I would find it very helpful for this error message to state the name of the key that is missing from the header.
There was a problem hiding this comment.
I believe that if this exception is raised leading to a failure to calculate a translated property, then you will end up with the property being reported in addition to this message.
| self._used_these_cards(long_key, lat_key) | ||
| break | ||
| else: | ||
| value = EarthLocation.of_site("CFHT") | ||
| return value | ||
|
|
||
| def to_detector_num(self): |
There was a problem hiding this comment.
It didn't exist before, but could you add a simple docstring?
There was a problem hiding this comment.
I was trying not to write repetitive docstrings when nothing is different to the base class. If you use pydoc or sphinx the documentation will turn up. @jonathansick I think I recall a conversation somewhere where we said it was okay to inherit docstrings.
There was a problem hiding this comment.
I wondered about this myself the other day, in a different context.
I reckon that for much of the codebase, at least while we're in development, the primary consumer of the docstring will be somebody who is directly modifying or — at least — looking at the code. Directing them to some other tool in order to read the API documentation seems unfortunate, so my instinct is that repetitive docstrings are actually helpful.
At the same time, though, I'm as leery of copying & pasting as the next person, so I'm not totally happy with that answer.
I'd definitely be interested in hearing ideas on best practice from other projects, although I do caution that we should use them as guidance rather than adopt them wholesale.
There was a problem hiding this comment.
For the translators embedded in the package I think it's reasonable for someone working on the package to understand what the standard properties in ObservationInfo mean without having to copy and paste everything (the docstrings are automatically constructed in the base class from the property definitions). The documentation for a particular translation doesn't help much then. If something a bit interesting is happening then I agree that a docstring should be added. If the consensus is that I should always add them then I will do so. It's probably the case that if a translator is external to this package it would be clearer to always add the docstring.
There was a problem hiding this comment.
I would generally prefer not to duplicate docstrings. A code comment indicating that a valid docstring can be found in a particular base class might be a good idea. If you're reading the docs by looking at the code itself, you're probably in a position to easily find docs in another file already; if you wanted effortless navigation you'd be reading the HTML.
There was a problem hiding this comment.
A code comment indicating that a valid docstring can be found in a particular base class might be a good idea
I very much like this idea.
There was a problem hiding this comment.
Putting in a code comment was our conclusion in Slack. I'll make a point of getting this into the Developer Guide.
| @@ -121,22 +131,53 @@ def to_observation_type(self): | |||
| return obstype | |||
|
|
|||
| def to_tracking_radec(self): | |||
There was a problem hiding this comment.
As above, this function could also use a docstring.
| return radec | ||
| if self.to_observation_type() == "science": | ||
| raise KeyError("Unable to determine tracking RA/Dec of science observation") | ||
| return None | ||
|
|
||
| def to_altaz_begin(self): |
There was a problem hiding this comment.
As above, this function could also use a docstring.
|
|
||
| def to_detector_exposure_id(self): | ||
| return self.to_exposure_id() * 36 + self.to_detector_num() | ||
|
|
||
| def to_pressure(self): |
There was a problem hiding this comment.
As above, this function could also use a docstring.
| exposure_time=0.0*u.s, | ||
| object="postflats-BIAS", | ||
| observation_id="ct4m20121211t220632", | ||
| observation_type="zero", |
There was a problem hiding this comment.
You don't want to test observation_type="science"?
There was a problem hiding this comment.
The other header tested in that file is a science observation.
timj
force-pushed
the
tickets/DM-16253
branch
from
d817aa8
to
2d0c110
Compare
No description provided.