Tickets/DM-40213 #137

gmegh · 2023-09-12T16:47:24Z

No description provided.

tribeiro

Here is an initial round of comments.

doc/version_history.rst

tribeiro · 2023-09-14T18:29:38Z

python/lsst/ts/standardscripts/maintel/closed_loop.py

+    M2_B20 = 49
+
+
+class ClosedLoop(salobj.BaseScript, metaclass=abc.ABCMeta):


Not sure this is an appropriate name for the script. I think usually we want to reflect an action performed by the control system. Maybe drop the "d" and call it CloseLoop? However, I think the intention here is to create a base script to perform close loop with both ComCam and LSSTCam. If this is the case, I'd call this BaseCloseLoop.

I see that you prepared the class to do it (by adding the metaclass), however there's nothing in the implementation on that regard. I think you want to split this one in 2 scripts; a base and a ComCam implementation, leaving the LSSTCam implementation for later. Or maybe even doing the LSSTCam already after we merge the PR in ts_observatory_control.

tribeiro · 2023-09-14T18:29:59Z

python/lsst/ts/standardscripts/maintel/closed_loop.py

+    FAM = 1
+
+
+class DOFComponent(enum.IntEnum):


Do you want to move this enum to ts_observatory_control as we spoke?

tribeiro · 2023-09-14T18:30:42Z

python/lsst/ts/standardscripts/maintel/closed_loop.py

+from lsst.ts import salobj
+from lsst.ts.observatory.control import RemoteGroup
+
+# TO-DO: Remove this import once the new LSSTCam is available


If you are making a generic implementation here, then, you don't want to import these here now. See my comment about splitting this into 2 scripts.

tribeiro · 2023-09-14T18:31:03Z

python/lsst/ts/standardscripts/maintel/closed_loop.py

+            descr=descr,
+        )
+
+        self.mtaos = RemoteGroup(


MTAOS is part of MTCS

tribeiro · 2023-09-14T18:36:33Z

python/lsst/ts/standardscripts/maintel/closed_loop.py

+
+        # The following attributes are set via the configuration:
+        self.filter = None
+        self.grating = None


The Main Telescope instruments have no grating

tribeiro · 2023-09-14T18:37:14Z

python/lsst/ts/standardscripts/maintel/closed_loop.py

+                description: De-focus to apply when acquiring the intra/extra focal images (mm).
+                type: number
+                default: 0.8
+              extra_focal_offset:


I don't think the MT has need of this

tribeiro · 2023-09-14T18:46:41Z

python/lsst/ts/standardscripts/maintel/closed_loop.py

+                    the change in magnification from moving the secondary mirror.
+                type: number
+                default: 0.0011
+              threshold:


Maybe instead if having a parameter for threshold and one for selecting the DoF, you could merge these two and make the DoF be a "dictionary" where users can specify the threshold. something like this:

dof: type: object description: >- Defines the DoF to use and their associated properties. Set the threshold to zero to disable a dof. properties: M2_dz: type: object properties: threshold: type: number M2_dx: type: object properties: threshold: type: number M2_dy: type: object properties: threshold: type: number M2_rx: type: object properties: threshold: type: number M2_ry: type: object properties: threshold: type: number . . .

Now, there's a question about how to handle defaults. Maybe we should talk about this in a video conf.

tribeiro · 2023-09-14T18:51:39Z

python/lsst/ts/standardscripts/maintel/closed_loop.py

+                await self.checkpoint(f"[{i + 1}/{self.max_iter}]: Taking image...")
+
+            # Setting visit_id's to none so run_cwfs will take a new dataset.
+            if self.mode == ClosedLoopMode.CWFS:


Instead of having each option in a if/else statement like this, I'd suggest you extract each "mode" into a function, then make a dictionary with the mode as key and the function as value. Then here you just do:

await self.operation_model_handlers[self.mode]()

where you would define self.operation_model_handlers in the `init method as something like:

self.operation_model_handlers = { ClosedLoopMode.CWFS: self.handle_cwfs_mode, ClosedLoopMode.FAM: self.handle_fam_mode, }

Then self.handle_cwfs_mode and self.handle_fam_mode will have the appropriate code extracted from here.

tribeiro · 2023-09-14T18:52:40Z

python/lsst/ts/standardscripts/maintel/closed_loop.py

+                self.mtaos.rem.mtaos.cmd_runWEP.set_start(
+                    visitId=intra_visit_id, extraId=extra_visit_id
+                )
+


Please, extract the following code into methods that explain what each step does.

tribeiro

Here some additional comments.

tribeiro · 2023-10-06T15:42:31Z

doc/version_history.rst

@@ -6,7 +6,10 @@
 Version History
 ===============

-.. towncrier release notes start
+v1.26.0


We are now using towncrier to manage the release notes. See: https://github.com/lsst-ts/ts_standardscripts/blob/develop/doc/news/README.rst

In short, you should not update this file anymore. Instead create a news fragment in the doc/news directory following the format suggested in the README above.

tribeiro · 2023-10-06T15:50:30Z

python/lsst/ts/standardscripts/base_close_loop.py

+                type: string
+                enum: ["cwfs", "fam"]
+                default: cwfs
+              rotation_angle:


I think you can just remove the rotator from the configuration. The software that needs to know about the rotator angle will get it from the correct source inside the MTAOS.

tribeiro · 2023-10-06T15:55:43Z

python/lsst/ts/standardscripts/base_close_loop.py

+                        f"[{i + 1}/{self.max_iter}]: Closed Loop converged."
+                    )
+
+                self.log.info("Closed Loop completed successfully!\n")


remove new line at the end

python/lsst/ts/standardscripts/base_close_loop.py

tribeiro · 2023-10-06T16:01:49Z

python/lsst/ts/standardscripts/base_close_loop.py

+
+            # If we haven't converged and apply_corrections is true,
+            # then we apply the corrections
+            elif self.apply_corrections:


I think you always want to apply the correction, even if you already converged. This way we don't trow away any iteration. I'd move this check to be before the convergence criteria check and make them independent if statements, e.g.;

if self.apply_corrections: ... if all(abs(dof_offset) < self.threshold): ...

you don't need the else step.

Also, if we are not applying the correction, do we want to continue taking data? we might want to break or return in that case, e.g.;

if self.apply_corrections: ... else: return

I think we do want to keep taking data if we are not applying the correction. The reason being that in that way we can see how the closed loop corrections evolve without applying them. We might be repositioning the telescope or checking the LUT while doing this

Ok, that's a good point. In this case, please, add a note in the max_iter description explaining that, if apply_corrections is False it will take this many pairs of images.

tribeiro · 2023-10-06T16:23:27Z

python/lsst/ts/standardscripts/maintel/close_loop_comcam.py

+            self.log.debug("Camera already defined, skipping.")
+
+    async def assert_mode_compatibility(self) -> None:
+        """Assert that the mode is compatible with ComCam."""


Please, document that is raises an exception.

tribeiro · 2023-10-06T16:30:02Z

python/lsst/ts/standardscripts/base_close_loop.py

+
+    @property
+    @abc.abstractmethod
+    def camera(self):


Can you add type annotation to these method? I notice below that you did ComCam | None and LSSTCam | None. Type-checking wise this will make our life harder. We currently don't have type checking on in this repo but looking forward I would like to add support for it.

I'd recommend you only return the camera type and raise an exception if the camera is None. Something like this:

@property @abc.abstractmethod def camera(self) -> BaseCamera: raise NotImplementedError()

Then for ComCam and LSSTCam you would do something like this:

@property def camera(self) -> BaseCamera: if self._comcam is not None: return self._comcam else: raise RuntimeError("Camera not defined.")

You can either use BaseCamera or ComCam as the return type in this case.

tribeiro · 2023-10-06T16:51:05Z

python/lsst/ts/standardscripts/maintel/close_loop_comcam.py

+        self._comcam = None
+
+    @property
+    def camera(self) -> ComCam | None:


See my comment above about the type and how to handle this

tribeiro · 2023-10-06T16:51:24Z

python/lsst/ts/standardscripts/maintel/close_loop_lsstcam.py

+        self._lsstcam = None
+
+    @property
+    def camera(self) -> LSSTCam | None:


See my comment above about the type

tribeiro · 2023-10-06T16:52:46Z

tests/test_base_close_loop.py

+import numpy as np
+from lsst.ts import standardscripts
+from lsst.ts.observatory.control.utils.enums import ClosedLoopMode
+from lsst.ts.standardscripts import BaseCloseLoop


BaseCloseLoop is an abstract class, I don't think you can unit test it. You will have to test the concrete implementations.

Cool, do I need to add two unit tests? lsstcam and comcam are basically the same, is it okay if I just have an lsstcam unit test?

only one is fine. However, you may want to check the executable for both.

tribeiro · 2023-10-07T05:45:48Z

python/lsst/ts/standardscripts/base_close_loop.py

+    This script is used to perform measurements of the wavefront error, then
+    propose dof offsets based on ts_ofc. The offsets are not applied
+    automatically and must be turned on by the user through
+    apply_corrections attribute.


Please, add a note here that if apply_corrections is off, the script will take a series of intra/extra focal data instead, and the number of pairs is the number of maximum iterations.

tribeiro · 2023-10-07T06:00:42Z

doc/version_history.rst

Please, make sure you reset all changes to this file.

tribeiro

Here some additional comments. I think once you address those we can probably close this one.

you may want to run

git checkout develop -- doc/version_history.rst

to make sure all your changes to this file are reset. Also, please, expand a bit the news fragment.

tribeiro · 2023-10-07T06:01:28Z

doc/news/DM-40213.feature.rst

@@ -0,0 +1,3 @@
+Add new base_close_loop.py script, and executable.


Please, explaining what this script does.

tribeiro · 2023-10-07T06:08:56Z

python/lsst/ts/standardscripts/base_close_loop.py

+                  - type: array
+                    items:
+                      type: string
+                      enum: [


I think you could dynamically generate this by doing something like:

enum: {[dof_name.name for dof_name in DOFName]}

and covert this to an f-string, by adding a "f" at the top, e.g.:

schema_yaml = f""" ...

tribeiro · 2023-10-07T06:10:45Z

tests/test_maintel_close_loop_lsstcam.py

+
+            assert all(self.state_0 == np.zeros(50))
+
+    async def test_executable(self):


Please, split this test in 2, e.g.;

async def test_executable_close_loop_lsstcam(self): ... async def test_executable_close_loop_comcam(self): ...

tribeiro

Looking good. Here some additional comments!

tribeiro · 2023-10-17T14:49:45Z

doc/news/DM-40213.feature.rst

@@ -0,0 +1,4 @@
+Add new base_close_loop.py script, and executable. This script allows to run the closed loop,


Please, make sure every phrase is in a single line, e.g. merge lines 1 and 2.

tribeiro · 2023-10-17T14:51:04Z

python/lsst/ts/standardscripts/base_close_loop.py

+        """Handle creating MTCS object and waiting for remote to start."""
+        if self.mtcs is None:
+            self.log.debug("Creating MTCS.")
+            mtcs_usage = None if self.add_remotes else MTCSUsages.DryTest


I think you can remove the mtcs_usage part here.

tribeiro · 2023-10-17T14:51:23Z

python/lsst/ts/standardscripts/base_close_loop.py

+    of pairs is the number of maximum iterations.
+    """
+
+    def __init__(self, index=1, add_remotes=True, descr="") -> None:


I think you can remove the add_remotes parameter.

tribeiro · 2023-10-17T14:53:07Z

python/lsst/ts/standardscripts/base_close_loop.py

+                maxItems: 50
+                default: {[0.004]*50}
+              max_iter:
+                description: Maximum number of iterations.


I think multiline description should be like:

description: >- Maximum number of iterations. Note, if apply_corrections is False, the script will take [max_iter] pairs of images.

See, for instance, how it is done in program below.

tribeiro · 2023-10-17T15:56:01Z

python/lsst/ts/standardscripts/maintel/close_loop_comcam.py

+        """Handle creating Camera object and waiting for remote to start."""
+        if self.camera is None:
+            self.log.debug("Creating Camera.")
+            comcam_usage = None if self.add_remotes else ComCamUsages.StateTransition


See my previous message about removing add_remotes

tribeiro · 2023-10-17T15:56:49Z

python/lsst/ts/standardscripts/maintel/close_loop_lsstcam.py

+        """Handle creating Camera object and waiting for remote to start."""
+        if self.camera is None:
+            self.log.debug("Creating Camera.")
+            camera_usage = None if self.add_remotes else LSSTCamUsages.StateTransition


See my previous comment about removing add_remotes

tribeiro · 2023-10-17T16:12:29Z

python/lsst/ts/standardscripts/base_close_loop.py

I think this module can be inside maintel since it is maintel specific.

tribeiro · 2023-10-17T16:14:16Z

tests/test_maintel_close_loop_lsstcam.py

+    standardscripts.BaseScriptTestCase, unittest.IsolatedAsyncioTestCase
+):
+    async def basic_make_script(self, index):
+        self.script = CloseLoopLSSTCam(index=index, add_remotes=False)


Instead of using add_remotes, set the value of the camera and TCS classes here with a DryTest usage, e.g.;

self.script = CloseLoopLSSTCam(index=index) self.script.mtcs = MTCS( domain=self.script.domain, intended_usage=MTCSUsages.DryTest, log=self.script.log, ) self.script._lsstcam = LSSTCam( domain=self.script.domain, intended_usage=LSSTCam.DryTest, log=self.script.log, )

tribeiro

Alright! this is looking pretty good. I left one inline comment in the news fragment.

One general comment, please, make sure all your command executions have a timeout, e.g. check that all start and set_start calls pass a timeout=<something>. Otherwise they may lock for ever if there is a communication issue. I think your STD_TIMEOUT might be short for some commands. I suggest you create a CMD_TIMEOUT=60 and use in all commands.. however the runWEP command might require something larger, like 120.

tribeiro · 2023-10-19T01:23:43Z

doc/news/DM-40213.feature.rst

@@ -0,0 +1,3 @@
+Add new base_close_loop.py script, and executable. This script allows to run the closed loop, that is, taking images, processing them, and apply ts_ofc corrections.


Make sure each phrase takes one line.
e.g.;

Add new base_close_loop.py script, and executable. This script allows to run the closed loop, that is, taking images, processing them, and apply ts_ofc corrections.

I think you may also want to add an empty line between each "feature", e.g.;

Add new base_close_loop.py script, and executable. This script allows to run the closed loop, that is, taking images, processing them, and apply ts_ofc corrections. Add new maintel/close_loop_comcam.py script, unit test, and executable. Add new maintel/close_loop_lsstcam.py script, unit test, and executable.

tribeiro

Looks good! Thanks!

gmegh force-pushed the tickets/DM-40213 branch 3 times, most recently from 63291fb to 9b17217 Compare September 13, 2023 18:23

gmegh requested a review from tribeiro September 13, 2023 18:23

tribeiro reviewed Sep 14, 2023

View reviewed changes

gmegh force-pushed the tickets/DM-40213 branch from 5ab267b to 93cfb52 Compare October 5, 2023 20:08

tribeiro reviewed Oct 6, 2023

View reviewed changes

gmegh requested a review from tribeiro October 7, 2023 03:29

tribeiro reviewed Oct 7, 2023

View reviewed changes

doc/version_history.rst Outdated

Copy link

Member

tribeiro Oct 7, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please, make sure you reset all changes to this file.

gmegh reacted with thumbs up emoji

tribeiro reviewed Oct 7, 2023

View reviewed changes

gmegh force-pushed the tickets/DM-40213 branch 2 times, most recently from bfe0495 to 04be8cb Compare October 9, 2023 00:07

tribeiro reviewed Oct 17, 2023

View reviewed changes

tribeiro mentioned this pull request Oct 17, 2023

Tickets/DM-40219 #144

Merged

gmegh requested a review from tribeiro October 18, 2023 04:53

gmegh force-pushed the tickets/DM-40213 branch 4 times, most recently from 002baff to f7f7209 Compare October 23, 2023 04:08

tribeiro reviewed Oct 23, 2023

View reviewed changes

gmegh added 2 commits October 24, 2023 10:12

Adding news for close_loop scripts

d7cd25a

Adding close_loop scripts, executables and tests

0a4386d

gmegh force-pushed the tickets/DM-40213 branch from 89eb532 to 0a4386d Compare October 24, 2023 01:12

tribeiro approved these changes Oct 24, 2023

View reviewed changes

gmegh merged commit 56636be into develop Oct 24, 2023
2 checks passed

gmegh deleted the tickets/DM-40213 branch October 24, 2023 03:32

		M2_B20 = 49


		class ClosedLoop(salobj.BaseScript, metaclass=abc.ABCMeta):

		@@ -0,0 +1,3 @@
		Add new base_close_loop.py script, and executable.


		assert all(self.state_0 == np.zeros(50))

		async def test_executable(self):

		@@ -0,0 +1,4 @@
		Add new base_close_loop.py script, and executable. This script allows to run the closed loop,

Tickets/DM-40213 #137

Tickets/DM-40213 #137

Conversation

gmegh commented Sep 12, 2023

tribeiro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tribeiro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tribeiro Oct 7, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tribeiro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tribeiro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tribeiro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tribeiro left a comment

Choose a reason for hiding this comment

tribeiro Oct 7, 2023 •

edited

Loading