Tickets/dm 44670 #141
Tickets/dm 44670 #141
Conversation
elanaku
force-pushed
the
tickets/DM-44670
branch
from
b006d51
to
ad07ee7
Compare
| await electrometer.cmd_setIntegrationTime.set_start( | ||
| intTime=integration_time | ||
| ) | ||
| await electrometer.cmd_performZeroCalib.start(timeout=self.long_timeout) |
There was a problem hiding this comment.
Should this be set_start or just start? @tribeiro
There was a problem hiding this comment.
just start.. we only use set_start when we need to pass command parameters. Note that timeout is not a command parameter.
| @@ -226,19 +253,21 @@ async def run_calibration_sequence( | |||
|
|
|||
| calibration_type = getattr(CalibrationType, str(config_data["calib_type"])) | |||
| if calibration_type == CalibrationType.WhiteLight: | |||
| calibration_wavelenghts = np.array([float(config_data["wavelength"])]) | |||
| calibration_wavelengths = np.array([float(config_data["wavelength"])]) | |||
| else: | |||
| wavelength = float(config_data["wavelength"]) | |||
| wavelength_width = float(config_data["wavelength_width"]) | |||
There was a problem hiding this comment.
Can we change wavelength_width to something like filter_width or band_width. I find this term confusing. And why not just have a wavelength_start and wavelength_end with resolution?
There was a problem hiding this comment.
I think this is a matter of personal preference. I find passing center wavelength + width more expressive. This is not a filter or band width, but rather the "scan width". I think we can certainly change these to something else if you think they will be more clear, but let's not do it as part of this ticket. Let's focus on what is being doing here instead.
Feel free to open a ticket for updating this though, and we can discuss in the ticket what would be a better solution.
There was a problem hiding this comment.
Thanks for the updates Elana. I have a few comments I hope you will take into consideration before I can approve the PR.
Also, can you please update the test_setup_electrometers in tests/auxtel/test_atcalsys.py to reflect the changes you've made? basically, make sure the commands you added are awaited as expected?
| for component_name in self.components_attr | ||
| if "electrometer" in component_name | ||
| ]: | ||
| await electrometer.cmd_setMode.set_start(mode=mode) |
There was a problem hiding this comment.
Please, add a timeout to all the calls to set_start here and in the next 3 lines. You can use timeout=self.long_timeout.
| ) | ||
|
|
||
| async def setup_electrometers( | ||
| self, mode: int = 1, range: float = -1, integration_time: float = 0.1 |
There was a problem hiding this comment.
do you really need the default values here? it seems like this is always getting values from config_data above. I suggest you remove the default values.
| async def setup_electrometers( | ||
| self, mode: int = 1, range: float = -1, integration_time: float = 0.1 | ||
| ) -> None: | ||
| """Setup all electrometers.""" |
There was a problem hiding this comment.
Please document the input parameters.
| await asyncio.gather( | ||
| task_setup_monochromator, | ||
| task_setup_latiss, | ||
| task_setup_monochromator, task_setup_latiss, task_setup_electrometer |
There was a problem hiding this comment.
This is a picky comment but do you mind adding a comma after task_setup_electrometer? This will cause black to format the code such that each task is in a separate line (like it was originally), thus reducing the signature change when we add new tasks.
There was a problem hiding this comment.
Here some additional comments.
I think this is mostly ready to go. However, before I approve it, please, check my latest comments and cleanup the git history.
Also, one note on the news fragment. Since you are doing more than one change, the way to do that is by creating different files, one for each change. This will also allow you to properly classify them. It works like this:
file doc/news/DM-44670.bugfix.rst
Update ATCalSys so that the filter scans have the correct wavelength range.
file doc/news/DM-44670.feature.rst
Update BaseCalSys.setup_electrometers to setup electrometer mode, range, and integration time from input parameters.
file doc/news/DM-44670.feature.1.rst
Update ATCalSys.prepare_for_flat to call setup_electrometer.
file doc/news/DM-44670.feature.2.rst
Update atcasys configuration such that the electrometer exposure time is similar to the camera exposure time and to include the additional electrometer configuration.
| @@ -309,12 +318,10 @@ async def _take_data( | |||
| exposures_done: asyncio.Future = asyncio.Future() | |||
|
|
|||
| fiber_spectrum_exposure_coroutine = self.take_fiber_spectrum( | |||
| exposure_time=fiber_spectrum_exposure_time, | |||
| exposures_done=exposures_done, | |||
| exposure_time=fiber_spectrum_exposure_time, exposures_done=exposures_done | |||
There was a problem hiding this comment.
please, undo this change here.
| ) | ||
| electrometer_exposure_coroutine = self.take_electrometer_scan( | ||
| exposure_time=electrometer_exposure_time, | ||
| exposures_done=exposures_done, | ||
| exposure_time=electrometer_exposure_time, exposures_done=exposures_done |
There was a problem hiding this comment.
please, undo this change here.
| async def setup_electrometers(self) -> None: | ||
| """Setup all electrometers.""" | ||
| async def setup_electrometers( | ||
| self, mode: int, range: float, integration_time: float |
There was a problem hiding this comment.
if mode is an enumeration, please, use the enumeration instead of an int.
There was a problem hiding this comment.
I think we do pass an int that is then turned into an enum by the controller in ts_electrometer (at least, that's my understanding - @parfa30 can confirm). Should I make a note of that? Should I call this an enum value?
There was a problem hiding this comment.
yes, we are always passing int when we send the commands. But this is a high level user interface. So, if there is an enumeration that represents the values, we should probably use it.
There was a problem hiding this comment.
yeah, I will add in the enumeration!
|
|
||
| Parameters | ||
| ---------- | ||
| mode : `int` |
There was a problem hiding this comment.
if this is an enumeration, please use the enum instead
parfa30
force-pushed
the
tickets/DM-44670
branch
2 times, most recently
from
5196040
to
79d2100
Compare
parfa30
force-pushed
the
tickets/DM-44670
branch
from
79d2100
to
85b02a7
Compare
| Electrometer measurement range. | ||
| """ | ||
| elec_mode = getattr(UnitToRead, mode) | ||
| elecMode = elec_mode.value |
There was a problem hiding this comment.
Please, merge these 2 lines into:
electrometer_mode = getattr(UnitToRead, mode).value
| for electrometer in [ | ||
| getattr(self.rem, component_name) | ||
| for component_name in self.components_attr | ||
| if "electrometer" in component_name | ||
| ]: | ||
| await electrometer.cmd_setMode.set_start( | ||
| mode=elecMode, |
There was a problem hiding this comment.
Replace this with electrometer_mode
parfa30
force-pushed
the
tickets/DM-44670
branch
from
72bb4af
to
e085b00
Compare
|
Made those changes and fixed a problem with the tests. Rebased with develop. |
elanaku
force-pushed
the
tickets/DM-44670
branch
from
e085b00
to
72bb4af
Compare
ATCalSys updated so that (1) the filter scans have the correct wavelength range, (2) that setup_electrometers takes inputs from the config file about the electrometer mode, range, and integration time, and (3) there is only one electrometer scan per exposure.