DM-40740: Create an obs_package to handle the fiber spectrographs #1
Conversation
There was a problem hiding this comment.
Thanks for letting me look. I have lots of questions but some of those might be related to me not understanding.
I would like to see some explicit translator tests so that there is a sense of more than one day being tested. I am worried about the OBSID being a simple integer of 21 since the OBSID has to be unique for every observation (look at the LATISS ones and you will see that the sequence number is combined with the telescope name and day obs).
There is an implication that this code should work for more than one fiber spectrograph but I don't see a plan for how that will happen since a generic FiberSpec instrument name is being used. [RHL notes: yes, we have fiberSpectrographs in both the auxTel and the Simonyi telescope calibration systems)
| def getCamera(cls): | ||
| # Constructing a YAML camera takes a long time but we rely on | ||
| # yamlCamera to cache for us. | ||
| # N.b. can't inherit as PACKAGE_DIR isn't in the class |
There was a problem hiding this comment.
I would be open to you patching obs_lsst to add a package_dir class property so you could avoid copying this method.
There was a problem hiding this comment.
sounds good - is it ok to push this to a future ticket to keep things simple for now?
| @cache_translation | ||
| def to_datetime_begin(self): | ||
| self._used_these_cards("DATE-BEG") | ||
| return Time(self._header["DATE-BEG"], scale="tai", format="isot") |
There was a problem hiding this comment.
It would be nice if this instrument also had MJD-BEG.
There was a problem hiding this comment.
We can get this added. Please file a ticket, and make sure that @parfa30 knows
Add ingestion test. Add test data. Update default setup. Update to do in ingestion test.
Update readme. Update readme. Change readme format. Update setup. Update readme. Update lint file. Update policy header. Add new lines. Remove unnecessary config file.
Update detector name. Update detector name in test.
aferte
force-pushed
the
tickets/DM-40740
branch
from
c918524 to
924193a
Compare
…trum constructor.
| def getDetector(self): | ||
| return self.detector | ||
|
|
||
| def getInfo(self): |
There was a problem hiding this comment.
Not something you need to do now, but in future type annotations would really help. You should consider whether you need any of the explicitly getter methods since we are moving away from them for Exposure.
| otherwise. | ||
| """ | ||
|
|
||
| return "INSTRUME" in header and header["INSTRUME"] in ["FiberSpectrograph.Broad"] |
There was a problem hiding this comment.
Did this get resolved? ie is the broad going to be FiberSpec and the other variants will get different names when they start to arrive?
| return self.to_exposure_time() | ||
|
|
||
| @staticmethod | ||
| def compute_exposure_id(dayobs, seqnum, controller=None): |
There was a problem hiding this comment.
How does this differ to the parent class implementation?
There was a problem hiding this comment.
The parent method requires a supported controller, but currently the controller in the header is not supported. We are postponing this update and all related header updates to DM-43041.
Fix typo. Complete gitignore.
Update to dos and serial number mapping. Update comments.
aferte
force-pushed
the
tickets/DM-40740
branch
from
02dc553 to
8fb4874
Compare
Also needs lsst/daf_butler#948