Skip to content
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

Merged
merged 13 commits into from Nov 16, 2023
Merged

Conversation

CWestICL
Copy link
Contributor

@CWestICL CWestICL commented Nov 15, 2023

Added a typing alias called TemperatureSequence that is the type Sequence[Decimal | numpy.float64]. This is now the return type for the get_temperatures() function in temperature_monitor_base.py, senecak107.py and dp9800.py.

Closes #401

@CWestICL CWestICL self-assigned this Nov 15, 2023
Copy link
Collaborator

@alexdewar alexdewar left a 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.ndarrays don't actually count as Sequences (numpy/numpy#2776).

I think probably the best thing to do is:

  1. Forget about trying to include a type for the elements and just replace uses of TemperatureSequence with plain Sequences (which is an alias for Sequence[Any])
  2. Suppress the mypy warning about ndarrays not being Sequences in senecak107.py. Even though they're technically not Sequences (there are a few required methods missing), they're close enough for our purposes

Other things:

  1. You forgot to update dummy_temperature_monitor.py
  2. 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.

Copy link

codecov bot commented Nov 16, 2023

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (0f75c7c) 75.64% compared to head (7899756) 75.67%.
Report is 2 commits behind head on main.

❗ Current head 7899756 differs from pull request most recent head eab229f. Consider uploading reports for the commit eab229f to get more accurate results

Files Patch % Lines
finesse/hardware/plugins/temperature/senecak107.py 60.00% 2 Missing ⚠️
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.
📢 Have feedback on the report? Share it here.

Copy link
Collaborator

@alexdewar alexdewar left a 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 Decimals, which is no longer true. Would you mind converting these arguments to be floats please? You'll also need to update the call site to cast the temperatures to floats too.

Copy link
Collaborator

@alexdewar alexdewar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! 😄

@CWestICL CWestICL merged commit 468b805 into main Nov 16, 2023
12 checks passed
@CWestICL CWestICL deleted the temperature-alias branch November 16, 2023 16:10
alexdewar pushed a commit that referenced this pull request Apr 17, 2024
TemperatureSequence alias for temperature monitors
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TemperatureSequence alias
2 participants