CATKIT-96: Reduce file IO overhead for sending commands to BMC #236
Conversation
Also fix typo in find_repo_location().
Also simplify dm image to command with boolean indexing.
catkit/hardware/boston/DmCommand.py
Outdated
| if flat_map_dm1 is None: | ||
| fname = CONFIG_INI.get("boston_kilo952", "flat_map_dm1") | ||
| flat_map_dm1 = fits.getdata(os.path.join(calibration_data_path, fname)) | ||
|
|
||
| if flat_map_dm2 is None: | ||
| fname = CONFIG_INI.get("boston_kilo952", "flat_map_dm2") | ||
| flat_map_dm2 = fits.getdata(os.path.join(calibration_data_path, fname)) |
There was a problem hiding this comment.
I would still load them conditionally to account for single DM usage. There's no strong reason to do so, only the weak example case of us messing around with a single DM and not even having the second file on disk.
There was a problem hiding this comment.
I'll do the same for the get_m_per_volt_map() function just below this one then.
| @@ -25,14 +25,19 @@ def find_package_location(package='catkit'): | |||
| return importlib.util.find_spec(package).submodule_search_locations[0] | |||
|
|
|||
|
|
|||
| def find_repo_location(package='cakit'): | |||
| def find_repo_location(package='catkit'): | |||
catkit/util.py
Outdated
| mask_path = os.path.join(find_package_location("catkit"), "hardware", "boston", "kiloCdm_2Dmask.fits") | ||
| mask = fits.open(mask_path)[0].data | ||
| return mask | ||
| if get_dm_mask.mask is None: |
There was a problem hiding this comment.
Let's do the following just to keep code contained to the func def, please.
if getattr(get_dm_mask, "mask", None) is None:
...
catkit/util.py
Outdated
| return get_dm_mask.mask | ||
|
|
||
|
|
||
| get_dm_mask.mask = None |
| # Flatten the mask using index952 | ||
| mask = catkit.util.get_dm_mask() | ||
| index952 = np.flatnonzero(mask) | ||
| mask = catkit.util.get_dm_mask().astype('bool') | ||
| number_of_actuators = np.count_nonzero(mask) | ||
|
|
||
| number_of_actuators_per_dimension = CONFIG_INI.getint('boston_kilo952', 'dm_length_actuators') | ||
| number_of_actuators = CONFIG_INI.getint("boston_kilo952", "number_of_actuators") | ||
| image = np.zeros((number_of_actuators_per_dimension, number_of_actuators_per_dimension)) | ||
| image[np.unravel_index(index952, image.shape)] = dm_command[:number_of_actuators] | ||
| image = np.zeros(mask.shape) | ||
| image[mask] = dm_command[:number_of_actuators] |
There was a problem hiding this comment.
👍 Good reduction! It took me way too long to even figure out what the old code was doing.
|
@ehpor RE
Yes, please, thanks. |
I also added a docstring.
|
Tested on hardware: |
| return flat_map_dm1 if dm_num == 1 else flat_map_dm2 | ||
| return flat_map_dm2 | ||
|
|
||
| raise ValueError('dm_num should be either 1 or 2.') |
There was a problem hiding this comment.
Not gonna ask you to redo this, but this sort of logic pattern is neater and more resilient to future changes when self-contained as an else to an elif, i.e.,
if dm_num == 1:
...
elif dm_num == 2:
...
else:
raiseThere was a problem hiding this comment.
I considered doing a dictionary approach too. That would have been even cleaner. PEP 634 is gonna be so good for this type of stuff too. I've been waiting for that for a long time.
This PR caches the flat maps and dm mask which were read every time a DM command was sent to the Boston. This reduces the timing:
Before this PR:
My machine on simulator: 6.4-7.0ms.
Hicat2 on hardware: 4.7-4.8 ms.
After this PR:
My machine on simulator: 1.0ms.
Hicat2 on hardware: 0.71-0.81 ms.