Skip DM command discretization if dac_bit_depth is set to none #138
Conversation
|
@ehpor I don't know how to parse a |
|
This works in the THD2 sims as expected. @steigersg do you want to take this for a spin in HiCAT simulations before merging? |
|
@ivalaginja the YAML syntax for the Python None is either |
| discretized_surface = total_surface | ||
|
|
||
| if dac_bit_depth is not None: | ||
| voltages = np.clip(voltages, 0, 1) |
There was a problem hiding this comment.
Should this be inside the if statement?
There was a problem hiding this comment.
It was added in #108 which introduced the discretization, but it's true that it has nothing to do with it a priori. Should I move it out?
There was a problem hiding this comment.
I'd take it out of the if-statement. It's unrelated. Otherwise, this looks fine to me, but we'll wait for @steigersg to give this a once-over.
Confirmed works on HiCAT simulations. Tried both not giving anything to |
| discretized_voltages = voltages | ||
| discretized_surface = total_surface | ||
|
|
||
| if dac_bit_depth is not None: |
There was a problem hiding this comment.
Think this can just be shortened to
if dac_bit_depth:
but that's more stylistic - otherwise it looks good to me!
There was a problem hiding this comment.
Good point, I will keep it as it is simply because in this case here it doesn't seem to make a difference.
There was a problem hiding this comment.
Not in general. In this case, as we are expecting dac_bit_depth to be either a positive non-zero integer or None, this would work. However, for dac_bit_depth = 0, this would unexpectedly not take the if-statement. Additionally, other types will do unexpected things as well. I like being explicit in these cases.
See the second bullet point of PEP8 programming recommendations for a more official reference.
There was a problem hiding this comment.
Actually, shouldn't this be done on the actual boston service as well? @ivalaginja Blocking this for now.
|
@ehpor I thought no, since the hardware discretizes in any case? Do we want to have software-imposed discretization before the command gets sent off to hardware? |
|
@ivalaginja The way we have it now, is that the voltages sent to the DM are never discretized. However, the voltages that are put on the Furthermore, diverging behaviour from sim vs hardware service is unwanted in general. Furthermore, I see turning off bit depth as a kinda "default" case, when you don't know the bit depth of your DM yet. Not that that last point has any impact on this specific implementation. |
|
Understood, working on it. |
|
Which means I will need a hardware test. I can do that in a couple of hours. |
|
Ok I will need to defer the testing to tomorrow after all 😬 |
|
I just tested this on hardware and the Boston service accepts an empty value ok. I also tested with values 14 and 1 which worked ok (with 1 making any sensible control impossible). |
There was a problem hiding this comment.
LGTM after the hardware test (which imo would not have been necessary). Does @RemiSoummer need to look at this as well? (I'm next to him, so I might ask him regardless.)
|
Def ask him. I'm glad I tested on hardware, the DMs are critical enough that I'm fine doing potentially useless tests. |
Fixes #137.