Tickets/dm 46179 Extend TCS readiness check to other image types beyond OBJECT #229
Conversation
iglesu
force-pushed
the
tickets/DM-46179
branch
from
f1ff55b
to
9398833
Compare
There was a problem hiding this comment.
I have a couple inline comments for you to consider. Mainly, you need to make sure you are limiting the resources when creating ATCS and MTCS classes by using the intended_usage argument.
Also, I am not sure I understand what you did the in the unit tests. Can you please try to follow the style that is already there instead?
doc/news/DM-46179.feature.rst
Outdated
| @@ -0,0 +1 @@ | |||
| Extend TCS readiness check to other image types beyond OBJECT. | |||
There was a problem hiding this comment.
Please make sure your news fragment explains what it is that you are doing. You should probably say something like:
Configure TCS synchronization to the following scripts:
- ...
Then list the scripts.
| @@ -46,8 +46,13 @@ class TakeImageLatiss(BaseTakeImage): | |||
| def __init__(self, index): | |||
| super().__init__(index=index, descr="Take images with AT Camera") | |||
|
|
|||
| self.atcs = ATCS(self.domain, log=self.log) | |||
There was a problem hiding this comment.
Please, limit the resources used by the class by specifying the intended_usage attribute. This should be something like
self.atcs = ATCS(self.domain, log=self.log, intended_usage= ATCSUsages.Slew)
you will need to import ATCSUsages as alongside ATCS above.
| @@ -48,8 +49,13 @@ def __init__(self, index): | |||
|
|
|||
| self.config = None | |||
|
|
|||
| self.mtcs = MTCS(self.domain, log=self.log) | |||
There was a problem hiding this comment.
Same comment as above for the auxtel script, but it is a lot more critical for MT, you need to limit the usages or it will use way too many resources. It should be something like:
self.mtcs = MTCS(self.domain, log=self.log, intended_usage=MTCSUsages.Slew)
a1d710a
to
642bcdc
Compare
Integrated the `tcs_ready_to_take_data` method across scripts responsible for taking on-sky images, specifically for image types: OBJECT, ENGTEST, CWFS, ACQ Configure TCS synchronization to the following script: - auxtel/daytime_checkout/slew_and_take_image_checkout.py - auxtel/take_image_latiss.py - maintel/take_image_comcam.py - maintel/take_image_lsstcam.py - maintel/take_triplet_comcam.py Added unit tests for the above scripts.
iglesu
force-pushed
the
tickets/DM-46179
branch
from
642bcdc
to
3ca3042
Compare
There was a problem hiding this comment.
You need to document this change in a news fragment. Please use a separate file, e.g. DM-46179.feature.1.rst`
| from lsst.ts.observatory.control.maintel.comcam import ComCam, ComCamUsages | ||
| from lsst.ts.observatory.control.maintel.mtcs import MTCSUsages |
There was a problem hiding this comment.
Can you please import this alongside MTCS please? You might have to change the import above to be something like
lsst.ts.observatory.control.maintel.mtcs import MTCS, MTCSUsages
| @@ -48,8 +49,13 @@ def __init__(self, index): | |||
|
|
|||
| self.config = None | |||
|
|
|||
| self.mtcs = MTCS(self.domain, log=self.log) | |||
There was a problem hiding this comment.
Please restrict the resources.
https://rubinobs.atlassian.net/browse/DM-46179
Before I keep going I want to make sure I'm not deviating from the intended development objectives. Specially for the unit tests. Thanks!