Tickets/DM-40956 Take Twilight Flats #146

elanaku · 2024-09-14T13:13:18Z

No description provided.

tribeiro

Here is an initial round of comments.

python/lsst/ts/externalscripts/auxtel/latiss_take_twilight_flats.py

tribeiro · 2024-09-26T14:44:46Z

python/lsst/ts/externalscripts/auxtel/latiss_take_twilight_flats.py

+        else:
+            self.log.debug("ATCS already defined, skipping.")
+
+    @property


Please, move this property definition so it is close to the tcs one.

python/lsst/ts/externalscripts/auxtel/latiss_take_twilight_flats.py

python/lsst/ts/externalscripts/maintel/take_twilight_flats_comcam.py

python/lsst/ts/externalscripts/maintel/take_twilight_flats_lsstcam.py

python/lsst/ts/externalscripts/maintel/take_twilight_flats_comcam.py

tribeiro · 2024-09-26T15:03:31Z

python/lsst/ts/externalscripts/auxtel/latiss_take_twilight_flats.py

+        -------
+        instrument_filter: `string`
+        """
+        filter = self.config.filter


filter is a valid python keyword:

In [1]: filter? Init signature: filter(self, /, *args, **kwargs) Docstring: filter(function or None, iterable) --> filter object Return an iterator yielding those items of iterable for which function(item) is true. If function is None, return the items that are true.

Please, don't shadow it here. Just use self.config.filter directly, you are not really gaining anything by doing this.

python/lsst/ts/externalscripts/auxtel/latiss_take_twilight_flats.py

tribeiro

Here some additional comments.

Also, it looks like the unit tests are failing because sumit_utils is not available in the container. You can make it available by adding lsst-sitcom/summit_utils to the list of extra_packages in the Jenkinsfile.

python/lsst/ts/externalscripts/base_take_twilight_flats.py

tribeiro · 2024-09-26T21:26:29Z

python/lsst/ts/externalscripts/base_take_twilight_flats.py

+
+        return schema_dict
+
+    @abc.abstractmethod


Consider having all the abstract method defined close together.

tribeiro · 2024-09-26T21:26:45Z

python/lsst/ts/externalscripts/base_take_twilight_flats.py

+        raise NotImplementedError()
+
+    async def offset_telescope(self):
+        """Abstract method to dither the camera if desired."""


Fix docstring.

tribeiro · 2024-09-26T21:29:27Z

python/lsst/ts/externalscripts/base_take_twilight_flats.py

+
+        await super().configure(config)
+
+        self.client = self.make_consdb_client()


I don't see this method defined anywhere here. Also, you already defined self.client = ConsDbClient() during initialization.

tribeiro · 2024-09-26T21:31:14Z

python/lsst/ts/externalscripts/base_take_twilight_flats.py

+        raise NotImplementedError()
+
+    @abc.abstractmethod
+    def get_instrument_filter(self) -> str:


Please make sure all the abstract methods are defined together.

tribeiro · 2024-09-26T22:02:58Z

python/lsst/ts/externalscripts/base_take_twilight_flats.py

+        # Take one 1s flat to calibrate the exposure time
+        self.log.info("Taking 1s flat to calibrate exposure time.")
+        exp_time = 1
+        await self.camera.take_flats(


Maybe this should be a take_acq instead? We probably don't want to use this image for flat field right?

tribeiro · 2024-09-26T22:05:35Z

python/lsst/ts/externalscripts/base_take_twilight_flats.py

+            exp_time = self.get_new_exptime(sky_counts, exp_time)
+
+            if exp_time > self.config.max_exp_time:
+                raise Exception(


Is this really a fail condition? it probably means the sky is too dark to take sky flats right?

I would suggest logging it as a warning and returning or break the loop. If you want to fail the script, please, raise RuntimeError instead of Exception.

tribeiro · 2024-09-26T22:06:27Z

python/lsst/ts/externalscripts/base_take_twilight_flats.py

+            if np.abs(self.config.dither) > 0:
+                self.offset_telescope()
+
+            await self.camera.take_flats(


I think we need to add support for sky flat type in the camera.

tribeiro · 2024-09-26T22:07:53Z

python/lsst/ts/externalscripts/base_take_twilight_flats.py

+        )
+
+        if (where_sun < self.config.min_sun) or (where_sun > self.config.max_sun):
+            raise Exception(


Please, don't raise Exception... either use RuntimeError or, in this case, an AssertionError.

tribeiro · 2024-09-26T22:08:45Z

python/lsst/ts/externalscripts/base_take_twilight_flats.py

+
+        return coords[arg]
+
+    def confirm_sun_location(self):


Consider renaming this method to assert_sun_location and then raise an AssertionError if the sun is not in the limits.

tribeiro · 2024-10-03T20:47:09Z

python/lsst/ts/externalscripts/base_take_twilight_flats.py

+        raise NotImplementedError()
+
+    @abc.abstractmethod
+    async def setup_instrument(self, ra, dec) -> str:


Can you rename this method to something that reflects what it is supposed to do now? maybe track_radec_and_setup_instrument? Also, you are not passing the instrument configuration, so, consider using the same strategy for ra and dec, in which case, I would suggest a name like track_and_setup_instrument

tribeiro · 2024-10-03T20:49:05Z

python/lsst/ts/externalscripts/base_take_twilight_flats.py

+
+    @abc.abstractmethod
+    async def setup_instrument(self, ra, dec) -> str:
+        """Abstract method to set the instrument filter.


It is also doing the slew/tracking now.

tribeiro · 2024-10-03T20:49:34Z

python/lsst/ts/externalscripts/base_take_twilight_flats.py

+
+    async def offset_telescope(self):
+        """Dither the camera between exposures if desired."""
+        await self.atcs.offset_azel(


tribeiro

Here is another round of comments..

tribeiro · 2024-10-04T20:02:17Z

python/lsst/ts/externalscripts/auxtel/latiss_take_twilight_flats.py

+        timeout = 30
+        query = f"SELECT * from cbd_latiss.visit1_quicklook where exposure_id = {self.latest_exposure_id}"
+        item = "post_isr_pixel_median"
+        sky_counts = self.client.wait_for_item_in_row(query, item, timeout)


Since this is a regular method and not a coroutine, you will need to run this in an executor. It should be something like this:

First, at the top:

import functools

then here you would do:

get_sky_counts = functools.partial(self.client.wait_for_item_in_row, query, item, timeout) sky_counts = await asyncio.get_running_loop().run_in_executor( None, get_sky_counts )

you might want to find the name of the parameters you are passing to wait_for_item_in_row and use them instead of relying on their ordering, e.g.:

get_sky_counts = functools.partial(self.client.wait_for_item_in_row, query=query, item=item, timeout=timeout)

note that I don't know if the names are correct, I just guessing.

tribeiro · 2024-10-04T20:04:38Z

python/lsst/ts/externalscripts/base_take_twilight_flats.py

+
+        return schema_dict
+
+    async def offset_telescope(self):


I feel like you can remove this method and just use the call inline.

tribeiro · 2024-10-04T20:10:31Z

python/lsst/ts/externalscripts/base_take_twilight_flats.py

+            f" The elevation of the Sun is {sun_coordinates[1]:.2f} deg"
+        )
+
+        if (sun_coordinates[1] < self.config.min_sun_elevation) or (


minor detail, but you can make this in a single statement with:

assert (sun_coordinates[1] < self.config.min_sun_elevation) or (sun_coordinates[1] > self.config.max_sun_elevation), f"Sun elevation {sun_coordinates} is outside appropriate elvation limits. Aborting."

tribeiro · 2024-10-04T20:13:41Z

python/lsst/ts/externalscripts/base_take_twilight_flats.py

+            sun_coordinates[1] > self.config.max_sun_elevation
+        ):
+            raise AssertionError(
+                f"Sun elevation {sun_coordinates} is outside appropriate elvation limits. Aborting."


There is a typo in "elvation" -> "elevation". But also, consider removing "Aborting" at the end and also logging what are the conditions. Something like:

f"Sun elevation {sun_coordinates[1]} is outside appropriate limits. " f"Must be below {self.config.min_sun_elevation} or above {self.config.max_sun_elevation}."

tribeiro · 2024-10-04T20:16:42Z

python/lsst/ts/externalscripts/base_take_twilight_flats.py

+
+        return target_radec
+
+    def get_empty_field(self, target, radius=5):


You need to convert this method to a coroutine and run the query in an executor (see below).

Otherwise calling this will block the async event loop and will look like the script has died.

tribeiro · 2024-10-04T20:48:33Z

python/lsst/ts/externalscripts/maintel/take_twilight_flats_lsstcam.py

+                    dec=self.config.dec,
+                )
+            ),
+            asyncio.create_task(self._wait_rotator_reach_filter_change_angle()),


you don't need this.

tribeiro · 2024-10-04T20:48:51Z

python/lsst/ts/externalscripts/maintel/take_twilight_flats_lsstcam.py

+            asyncio.create_task(
+                self.mtcs.slew_icrs(
+                    ra=self.config.ra,
+                    dec=self.config.dec,


you need to pass rot_type=RotType.Physical.

tribeiro · 2024-10-04T20:49:04Z

python/lsst/ts/externalscripts/maintel/take_twilight_flats_lsstcam.py

+
+        await self.lsstcam.setup_filter(filter=self.config.band_filter)
+
+    async def _wait_rotator_reach_filter_change_angle(self):


You don't need this.

python/lsst/ts/externalscripts/maintel/take_twilight_flats_lsstcam.py

tribeiro · 2024-10-04T20:52:49Z

tests/auxtel/test_latiss_take_twilight_flats.py

+try:
+    from lsst.summit.utils import ConsDbClient
+
+    CONSDB_AVAILABLE = True


Instead of doing this, consider mocking the behavior. For example, take a look at: https://github.com/lsst-ts/ts_standardscripts/blob/65fd9b9e57cdb0ef93a658f99f89975708767c32/tests/test_base_block_script.py#L76-L78

You can mock the behavior of method calls by patching them.

tribeiro · 2024-10-04T23:30:26Z

python/lsst/ts/externalscripts/maintel/take_twilight_flats_lsstcam.py

+                self.mtcs.slew_icrs(
+                    ra=self.config.ra,
+                    dec=self.config.dec,
+                    rot_type=RotType.PhysicalSky,


This one needs to be RotType.Physical instead. this will ensure the rotator is kept at the fixed position.

edennihy

Changes approved in the interest of getting these merged. Expect some improvement after on-sky testing with ComCam.

elanaku requested a review from tribeiro September 14, 2024 13:13

tribeiro reviewed Sep 26, 2024

View reviewed changes

elanaku force-pushed the tickets/DM-40956 branch from 34d630a to ec3ebd8 Compare September 27, 2024 01:57

tribeiro reviewed Oct 3, 2024

View reviewed changes

tribeiro reviewed Oct 4, 2024

View reviewed changes

elanaku force-pushed the tickets/DM-40956 branch from ad98282 to c227238 Compare October 9, 2024 13:04

elanaku force-pushed the tickets/DM-40956 branch from 31694f7 to b43f822 Compare October 22, 2024 05:21

elanaku added 5 commits October 22, 2024 19:12

Add take twilight flats with latiss script

669bb87

Add TakeTwilightFlatsComCam script

3a04286

Add TakeTwilightFlatsLSSTCam script

b750cc7

Update inits

935a21b

Add news fragment

9ae2427

elanaku force-pushed the tickets/DM-40956 branch from 6c8ded4 to 9ae2427 Compare October 22, 2024 19:15

edennihy self-requested a review October 22, 2024 19:22

edennihy approved these changes Oct 22, 2024

View reviewed changes

elanaku merged commit f75e885 into develop Oct 22, 2024
2 checks passed

elanaku deleted the tickets/DM-40956 branch October 22, 2024 19:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tickets/DM-40956 Take Twilight Flats #146

Tickets/DM-40956 Take Twilight Flats #146

elanaku commented Sep 14, 2024

tribeiro left a comment

tribeiro Sep 26, 2024

tribeiro Sep 26, 2024

tribeiro left a comment

tribeiro Sep 26, 2024

tribeiro Sep 26, 2024

tribeiro Sep 26, 2024

tribeiro Sep 26, 2024

tribeiro Sep 26, 2024

tribeiro Sep 26, 2024

tribeiro Sep 26, 2024

tribeiro Sep 26, 2024

tribeiro Sep 26, 2024

tribeiro Oct 3, 2024 •

edited

Loading

tribeiro Oct 3, 2024

tribeiro Oct 3, 2024

tribeiro left a comment

tribeiro Oct 4, 2024

tribeiro Oct 4, 2024

tribeiro Oct 4, 2024

tribeiro Oct 4, 2024

tribeiro Oct 4, 2024

tribeiro Oct 4, 2024

tribeiro Oct 4, 2024

tribeiro Oct 4, 2024

tribeiro Oct 4, 2024

tribeiro Oct 4, 2024

edennihy left a comment


		await super().configure(config)

		self.client = self.make_consdb_client()


		return target_radec

		def get_empty_field(self, target, radius=5):


		await self.lsstcam.setup_filter(filter=self.config.band_filter)

		async def _wait_rotator_reach_filter_change_angle(self):

Tickets/DM-40956 Take Twilight Flats #146

Tickets/DM-40956 Take Twilight Flats #146

Conversation

elanaku commented Sep 14, 2024

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

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 Oct 3, 2024 • 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

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

edennihy left a comment

Choose a reason for hiding this comment

tribeiro Oct 3, 2024 •

edited

Loading