New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
TemperatureSequence alias for temperature monitors #416
Conversation
for more information, see https://pre-commit.ci
…eLondon/FINESSE into temperature-alias
for more information, see https://pre-commit.ci
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So it looks like there are actually two mypy
errors that are breaking things: the one in manage_devices.py
that we talked about (#417) and another one in your code:
______________ finesse/hardware/plugins/temperature/senecak107.py ______________
152: error: Incompatible return value type (got "ndarray[Any, Any]", expected "Sequence[Union[Decimal, floating[_64Bit]]]") [return-value]
Bizarrely, one of them is being picked up by the pre-commit
hook and the other by the pytest
plugin. (So to see the other one you'll have to run pytest finesse
.)
After digging into this a bit, it seems that the solution isn't as clean as I thought, because, annoyingly, np.ndarray
s don't actually count as Sequence
s (numpy/numpy#2776).
I think probably the best thing to do is:
- Forget about trying to include a type for the elements and just replace uses of
TemperatureSequence
with plainSequence
s (which is an alias forSequence[Any]
) - Suppress the
mypy
warning aboutndarray
s not beingSequence
s insenecak107.py
. Even though they're technically notSequence
s (there are a few required methods missing), they're close enough for our purposes
Other things:
- You forgot to update
dummy_temperature_monitor.py
- You should also update the places in the frontend code where the temperature arrays are consumed (hint: search for occurrences of
list[Decimal]
)
It's probably easiest if we wait until PR #418 is merged before you try to tackle this though.
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #416 +/- ##
==========================================
+ Coverage 75.64% 75.67% +0.02%
==========================================
Files 55 55
Lines 2792 2795 +3
==========================================
+ Hits 2112 2115 +3
Misses 680 680 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
There's just one more change I've noticed that needs to be made on the frontend. Atm, the _update_figure()
method in temp_control.py
assumes its inputs are Decimal
s, which is no longer true. Would you mind converting these arguments to be float
s please? You'll also need to update the call site to cast the temperatures to float
s too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! 😄
TemperatureSequence alias for temperature monitors
Added a typing alias called
TemperatureSequence
that is the typeSequence[Decimal | numpy.float64]
. This is now the return type for theget_temperatures()
function intemperature_monitor_base.py
,senecak107.py
anddp9800.py
.Closes #401