Tickets/dm 45406 #137
Tickets/dm 45406 #137
Conversation
dsanmartim
force-pushed
the
tickets/DM-45406
branch
from
70acaa1 to
0794d17
Compare
|
|
||
| # Ensure flats_exp_times is provided in pairs | ||
| if len(config.flats_exp_times) % 2 != 0: | ||
| raise ValueError("Flats exposure times must be provided in pairs.") |
| # Check that each pair in flats_exp_times contains equal values | ||
| for i in range(0, len(config.flats_exp_times), 2): | ||
| if config.flats_exp_times[i] != config.flats_exp_times[i + 1]: | ||
| raise ValueError( |
| if len(config.flats_exp_times) % 2 != 0: | ||
| raise ValueError("Flats exposure times must be provided in pairs.") | ||
|
|
||
| # Check that each pair in flats_exp_times contains equal values |
There was a problem hiding this comment.
Why are you enforcing it this way? Why not let the user provide a single value and take twice the number of exposures?
| isinstance(config.interleave_darks["dark_exp_times"], list) | ||
| and config.interleave_darks["n_darks"] | ||
| ): | ||
| raise ValueError( |
There was a problem hiding this comment.
Should be RuntimeError. Also, why not just ignore this altogether? just log a warning saying that n_darks are going to be ignored because dark_exp_times is an array.
| """ | ||
| try: | ||
| electrometer_mode = getattr(UnitToRead, mode).value | ||
| except AttributeError: |
There was a problem hiding this comment.
Why convert an AttributeError into a ValueError. If anything (which I don't think is necessary) convert it into a RuntimeError... But I would just not this at all. Just do
electrometer_mode = getattr(UnitToRead, mode).value
| electrometer_mode = getattr(UnitToRead, mode).value | ||
| except AttributeError: | ||
| raise ValueError(f"Invalid electrometer mode: {mode}") | ||
|
|
There was a problem hiding this comment.
What if the electrometer is not defined? Above you only call configure_electrometer if electrometer_scan is passed to the configuration.
I would add an assert self.electrometer is not None here. But also, if you always need an electrometer make sure you make electrometer_scan required.
if hasattr(config, "electrometer_scan"):
await self.configure_electrometer(config.electrometer_scan["index"])
There was a problem hiding this comment.
Ok. But the idea is to make it optional.
There was a problem hiding this comment.
And the code you suggested is already done in the he configure method:
if hasattr(config, "electrometer_scan"):
await self.configure_electrometer(config.electrometer_scan["index"])
| exp_time + self.extra_scan_time | ||
| ) | ||
|
|
||
| flat_task = self.camera.take_imgtype( |
There was a problem hiding this comment.
Why not use take_flats? you know this is going to be a flat.
| f"Taking flat {j+1}/2 with exposure time: {exp_time} " | ||
| f"seconds." | ||
| ) | ||
| await self.camera.take_imgtype( |
| f"Taking dark {k+1}/{len(self.config.interleave_darks['dark_exp_times'])} " | ||
| f"for pair {i // 2 + 1} of {len(self.config.flats_exp_times) // 2}." | ||
| ) | ||
| await self.camera.take_imgtype( |
| assert self.script.config.interleave_darks["n_darks"] == 2 | ||
| assert self.script.config.interleave_darks["dark_exp_times"] == [30, 30] | ||
|
|
||
| async def test_invalid_config(self): |
There was a problem hiding this comment.
Maybe give a hint of what is invalid about it.
There was a problem hiding this comment.
I was testing an invalid electrometer mode, but this will be an enum in the schema.
dsanmartim
force-pushed
the
tickets/DM-45406
branch
from
f692253 to
d2e3197
Compare
Hi Tiago. Here is a draft pull request for your review.