Tickets/dm 37305 #80

patrickingraham · 2023-01-18T04:45:19Z

No description provided.

dsanmartim

Hello Patrick,

I'm finally sending my comments on this PR. It was a long way until I was able to understand the scripts (and all the infrastructure behind them, like the Base Scripts, the containers, the testing, CSCs, etc) and get to the point that I could make comments, but the path to it was also really rewarding and motivating. Most of my comments are in the spirit of making questions on how things will work and a few of them can be seen as suggestions that will hopefully be valuable ones.

Note that I'm submitting the review as "Comments" only. I look forward to seeing what comes next and to see how this script will evolve until it is ready to be merged.

Thanks,
David Sanmartim.

tests/data/auxtel/test_sequence1.yaml

dsanmartim · 2023-02-07T23:16:37Z

python/lsst/ts/externalscripts/data/scripts/auxtel/latiss_take_flats.py

+
+import asyncio
+
+from lsst.ts.externalscripts.auxtel.build_pointing_model import BuildPointingModel


Suggested change

from lsst.ts.externalscripts.auxtel.build_pointing_model import BuildPointingModel

from lsst.ts.externalscripts.auxtel.latiss_take_flats import LatissTakeFlats

The use of a commit suggestion is nice, but mostly not necessary. Incorporating commit changes in GH is sort of asking for trouble.
In the future, feel free to say "should be LatissTakeFlats" and we'll fix it.

Ok. And sorry for that.

dsanmartim · 2023-02-07T23:17:02Z

python/lsst/ts/externalscripts/data/scripts/auxtel/latiss_take_flats.py

+
+from lsst.ts.externalscripts.auxtel.build_pointing_model import BuildPointingModel
+
+asyncio.run(BuildPointingModel.amain())


Suggested change

asyncio.run(BuildPointingModel.amain())

asyncio.run(LatissTakeFlats.amain())

I'm not sure if this is related, but the test_executable method from test_latiss_take_flats.py is failing in my container. I also noticed that this test appears as Fixed in Jenkins, although it keeps failing to me. More comments in the test_latiss_take_flats module session.

So this passes in my container and passes in Jenkins (as you said).... but i'll read your comments below.

dsanmartim · 2023-02-07T23:45:05Z

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

+        )
+
+        # Check that the LATISS filter/grating combination is valid
+        if latiss_filter == "SDSSr_65mm" and latiss_grating == "empty_1":


I wonder if wouldn't be better to have a default sequence for each of the available filters at any given time (of course, after defining what the default parameters are). As the sequence might be really big, maybe this method should load the steps from a sort of a default_sequence.yaml file.

Yes! That's the plan!
We need to establish configs for each filter+grating combination.
Question for bonus points. Where do you think those sequence files belong?

Good question. I see that a default sequence for the test cases lives inside the tests/data/auxtel. I can also see that there is a data/scripts folder for the executable scripts, but I don't see a similar data folder inside the main scripts folder (/externalscripts/auxtel). I was looking around (e.g. ts_standardscripts) and could not find a similar case, so not much in favor of creating a data folder inside the main scripts folder. Maybe it could live in the lsst-ts/ts_config_ocs, where the scheduler/survey config files live.

dsanmartim · 2023-02-07T23:51:56Z

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

+                "fs_exp_time": 1,
+                "fs_n_exp": 1,
+                "em_exp_time": 5,


Shouldn't fs_exp_time and em_exp_time values be equal? If so, maybe we should check this somewhere when a custom sequence is provide. Maybe not now, as we still don't know what the input parameters will look like, but in the future.

So the electrometer and fiber spectrographs have different sensitivities, especially as a function of wavelength. This means that each system receives different count rates and basically saturates at different times. So for a given flat, which for example sake is 30s long, it's possible that we'll take ten 3s exposures with the fiber spectrograph, and 3 10s scans with the electrometer.

dsanmartim · 2023-02-08T03:28:15Z

tests/auxtel/test_latiss_take_flats.py

+        # valid sequence, using the default
+
+        async with self.make_script():
+            # Try configure with invalid sequence data.


Is this comment valid here?

dsanmartim · 2023-02-08T03:36:30Z

tests/auxtel/test_latiss_take_flats.py

+            # indication simulation for s3 bucket.
+            await self.script.arun(simulation_mode=1)
+
+    async def test_sequence_custom(self):


I found this method a little bit redundant with the test_configure method, as a substantial portion of it is repeats the configure test executed there. Wouldn't it be better to have just one test case then?

I understand that the idea is to have isolated and independent methods, but at same time, we usually don’t like to repeat code, so I’m not really sure on what to do here, but wanted to raise that question.

Fair comment. So there is overlap. I think what happened is that when developing this I started with the test_configure bit which is very fast to run then wrote the second which actually executes the script, so it's much longer.

I suppose the proper thing is to remove the bit in the test_configure method.

dsanmartim · 2023-02-08T03:37:53Z

tests/auxtel/test_latiss_take_flats.py

+        Mocks the taking of fiber spectrograph images
+        """
+
+        # FIXME TO SUPPORT MULTIPLE IMAGES


I just want to say that I'm curious on how to implement this :)

so the real answer is to create a high-level controller for this CSC that takes the multiple images, then just mock that 😃 This is the type of thing that will get ripped out of this script in the future.

dsanmartim · 2023-02-08T03:53:25Z

tests/auxtel/test_latiss_take_flats.py

+    async def test_executable(self):
+        scripts_dir = externalscripts.get_scripts_dir()
+        script_path = scripts_dir / "auxtel" / "latiss_take_flats.py"
+        logger.debug(f"Checking for script in {script_path}")
+        await self.check_executable(script_path)


I'm running all the test cases in my docker container and all but this one (test_executable) are passing. First, I thought it would be related to the scripts/auxtel/latiss_take_flats.py since it was not configured correctly. Then, I fixed it as suggested in the corresponding review comment. However, the failure did not disappear, raising a TimeoutError as shown below. Interestingly, Jenkins says that the test_executable method was "Fixed", so I assume it was failing at some point but then it got fixed. I just don’t understand why it fails when running inside the container.

====================================================================== ERROR: test_executable (test_latiss_take_flats.TestLatissTakeFlats) ---------------------------------------------------------------------- Traceback (most recent call last): File "/opt/lsst/software/stack/conda/envs/lsst-scipipe-5.1.0/lib/python3.10/asyncio/tasks.py", line 456, in wait_for return fut.result() asyncio.exceptions.CancelledError The above exception was the direct cause of the following exception: Traceback (most recent call last): File "/opt/lsst/software/stack/conda/envs/lsst-scipipe-5.1.0/lib/python3.10/unittest/async_case.py", line 64, in _callTestMethod self._callMaybeAsync(method) File "/opt/lsst/software/stack/conda/envs/lsst-scipipe-5.1.0/lib/python3.10/unittest/async_case.py", line 87, in _callMaybeAsync return self._asyncioTestLoop.run_until_complete(fut) File "/opt/lsst/software/stack/conda/envs/lsst-scipipe-5.1.0/lib/python3.10/asyncio/base_events.py", line 649, in run_until_complete return future.result() File "/opt/lsst/software/stack/conda/envs/lsst-scipipe-5.1.0/lib/python3.10/unittest/async_case.py", line 101, in _asyncioLoopRunner ret = await awaitable File "/opt/lsst/tssw/ts_externalscripts/tests/auxtel/test_latiss_take_flats.py", line 261, in test_executable await self.check_executable(script_path) File "/opt/lsst/tssw/ts_standardscripts/python/lsst/ts/standardscripts/base_script_test_case.py", line 123, in check_executable state = await remote.evt_state.next(flush=False, timeout=MAKE_TIMEOUT) File "/opt/lsst/tssw/ts_salobj/python/lsst/ts/salobj/topics/read_topic.py", line 657, in next return await self._next(timeout=timeout) File "/opt/lsst/tssw/ts_salobj/python/lsst/ts/salobj/topics/read_topic.py", line 669, in _next return await asyncio.wait_for(self._next_task, timeout=timeout) File "/opt/lsst/software/stack/conda/envs/lsst-scipipe-5.1.0/lib/python3.10/asyncio/tasks.py", line 458, in wait_for raise exceptions.TimeoutError() from exc asyncio.exceptions.TimeoutError ---------------------------------------------------------------------- Ran 1 test in 155.642s FAILED (errors=1)

Hard to tell what the issue is from that. Did you run the test in debug mode?

No. I did not. but I'm having same issue when running other test cases (e.g, test_correct_pointing.pyP) . So, I'll try to understand what is happening in my container.

dsanmartim · 2023-02-08T04:06:18Z

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

+        # Prepare summary table for writing
+        await self.prepare_summary_table()
+
+        for i, self.step in enumerate(self.sequence):


What happens if the script fails in the middle of the sequence? Should the script be run from the beginning again or maybe should it be run with an updated schema (in case a custom sequence is provided) that will take that into account?

If a default sequence is provided, then I guess that the solution would be "run everything again", right? Can somehow the summary_table be used to keep track of what was already done, so we can recover the script from that point?

… the atcalsys class.

Add sequence option to the configuration to pass more complex sequences.

…mation is preserved

…se integers for the time being. Also modify config to support custom sequences.

patrickingraham requested a review from dsanmartim January 18, 2023 04:57

patrickingraham force-pushed the tickets/DM-37305 branch 2 times, most recently from a0df3d9 to 919dca3 Compare January 25, 2023 00:17

dsanmartim reviewed Feb 8, 2023

View reviewed changes

patrickingraham force-pushed the tickets/DM-37305 branch 2 times, most recently from 7f4d0cf to 7cdd000 Compare February 9, 2023 05:46

Patrick Ingraham added 16 commits May 14, 2024 13:52

Initial push. Completely untested.

0e934c2

configure and script tests now passing.

7196ade

Changed tests to mock script functions, which will eventually go into…

bec78f8

… the atcalsys class.

tests now passing (I think)

3051d19

all tests passing. cleaned up flake8 etc.

34edb60

Updates based on testing. Script now runs to completion correctly

ba54575

update tests to be consistent with script that works on hardware.

a021243

Add sequence option to the configuration to pass more complex sequences.

now functional. Also had to remove obsnote and reason so header infor…

f45edb5

…mation is preserved

fix bug with input sequences

e2453c8

add debug message

ae24447

Monochromator grating enums are known to be incorrect, so modify to u…

4fa550b

…se integers for the time being. Also modify config to support custom sequences.

change n_flats to nflats in the script.

02cdc91

fix typo

fad754f

try to update perms on latiss_take_flats.py

ac8ea6c

Addressed a few PR comments.

0e80ba1

minor docstring fix

9cd2eee

dsanmartim force-pushed the tickets/DM-37305 branch from a5edde2 to 9cd2eee Compare May 14, 2024 17:53

Run black + isort

2daf684

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tickets/dm 37305 #80

Tickets/dm 37305 #80

patrickingraham commented Jan 18, 2023

dsanmartim left a comment •

edited

Loading

dsanmartim Feb 7, 2023

patrickingraham Feb 8, 2023

dsanmartim Feb 9, 2023

dsanmartim Feb 7, 2023

dsanmartim Feb 7, 2023

patrickingraham Feb 8, 2023

dsanmartim Feb 7, 2023

patrickingraham Feb 8, 2023

dsanmartim Feb 10, 2023

dsanmartim Feb 7, 2023

patrickingraham Feb 8, 2023

dsanmartim Feb 8, 2023

dsanmartim Feb 8, 2023

patrickingraham Feb 9, 2023

dsanmartim Feb 8, 2023

patrickingraham Feb 9, 2023

dsanmartim Feb 8, 2023

patrickingraham Feb 9, 2023

dsanmartim Feb 10, 2023

dsanmartim Feb 8, 2023


		import asyncio

		from lsst.ts.externalscripts.auxtel.build_pointing_model import BuildPointingModel

	from lsst.ts.externalscripts.auxtel.build_pointing_model import BuildPointingModel
	from lsst.ts.externalscripts.auxtel.latiss_take_flats import LatissTakeFlats


		from lsst.ts.externalscripts.auxtel.build_pointing_model import BuildPointingModel

		asyncio.run(BuildPointingModel.amain())

	asyncio.run(BuildPointingModel.amain())
	asyncio.run(LatissTakeFlats.amain())

Tickets/dm 37305 #80

Are you sure you want to change the base?

Tickets/dm 37305 #80

Conversation

patrickingraham commented Jan 18, 2023

dsanmartim left a comment • edited Loading

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

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

dsanmartim left a comment •

edited

Loading