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.

Sign up for GitHub

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

Jump to bottom

Include a replacement for " Celsius" as the unit #333

Merged
merged 2 commits into from Mar 3, 2022
Merged

Include a replacement for " Celsius" as the unit #333

merged 2 commits into from Mar 3, 2022

Conversation

trappitsch
Copy link
Contributor

See issue #331, it seems like tset? returns Celsius as the unit.

@trappitsch
Include a replacement for " Celsius" as the unit
af41e4e 
See issue instrumentkit#331, it seems like `tset?` returns ` Celsius` as the unit.
@coveralls
Copy link

coveralls commented Mar 1, 2022 •
edited

Coverage Status

Coverage increased (+8.0e-05%) to 99.483% when pulling 36d2baf on trappitsch:bf_tc200 into 408ef66 on Galvant:main.

@trappitsch
Copy link
Contributor Author

Added a separate test to check that Celsius as the unit is replaced properly. The other tests ensure that C is replaced properly as well (replacement order matters).

scasagrande
scasagrande reviewed Mar 2, 2022
View reviewed changes
instruments/thorlabs/tc200.py
.replace(" Celsius", "")
.replace(" C", "")
.replace(" F", "")
.replace(" K", "")
)
return u.Quantity(float(response), u.degC)
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we update this to parse the units instead of just throwing them away?

Copy link
Contributor Author

@trappitsch trappitsch Mar 3, 2022 •
edited

Choose a reason for hiding this comment

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

According to the manual, section 6.3.2:

All temperature inputs and read backs are in °C only, regardless of what the display units are for the front panel LCD of the TC200.

I assume it would never return other units than C, but then again... didn't except Celsius either.

Copy link
Contributor

Choose a reason for hiding this comment

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

😂 fair enough! haha

@scasagrande
Copy link
Contributor

Just update your branch and we are good to go 💃

@trappitsch
Copy link
Contributor Author

I noticed that at some point when browsing through the manual, it's strange indeed since the instrument can display other units... oh well 😄
Branch should be updated, sorry, I missed that before. With all the recent work I must have fallen a few commits behind on my fork...

@scasagrande scasagrande merged commit 9c167d4 into instrumentkit:main Mar 3, 2022
@scasagrande scasagrande mentioned this pull request Mar 3, 2022
Closed
@trappitsch trappitsch deleted the bf_tc200 branch March 3, 2022 17:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@scasagrande scasagrande scasagrande approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@trappitsch @coveralls @scasagrande