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

Promote unit of parameter to be a property #3929

Merged
merged 4 commits into from
Feb 16, 2022

Conversation

jenshnielsen
Copy link
Collaborator

@jenshnielsen jenshnielsen commented Feb 15, 2022

Inspired by the typing hacks needed in #3916 to make unit a property

This eliminates the need for those hacks See https://github.com/QCoDeS/Qcodes/pull/3916/files#diff-5478d126afe3357045220d8be1ef9c6859ee8427f28984ff8d169b9ab1b75902R126

  • Should we do this for other _BaseParameter subclasses. No need

@codecov
Copy link

codecov bot commented Feb 15, 2022

Codecov Report

Merging #3929 (1fe6b12) into master (cb7fbde) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #3929   +/-   ##
=======================================
  Coverage   66.07%   66.07%           
=======================================
  Files         228      228           
  Lines       31153    31160    +7     
=======================================
+ Hits        20583    20590    +7     
  Misses      10570    10570           

Copy link
Contributor

@astafan8 astafan8 left a comment

Choose a reason for hiding this comment

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

making a property is nice, but if it's only about typing, would annotating the unit attribute in __init__ where it's defined suffice?

about adding unit property to _BaseParameter - i don't think it's particularly useful since most of the "useful" parmaeters inherit from Parameter, and the only two that inherit from _BaseParameter are Array and Multi parameter , where the first one is sort of deprecated and the latter one has concept of units with s as opposed to unit.

qcodes/instrument/parameter.py Show resolved Hide resolved
qcodes/instrument/parameter.py Outdated Show resolved Hide resolved
@jenshnielsen
Copy link
Collaborator Author

making a property is nice, but if it's only about typing, would annotating the unit attribute in init where it's defined suffice?

The reason for doing this it to allow a subclass to do the same without having to ignore type checking errors. See https://github.com/QCoDeS/Qcodes/pull/3916/files#diff-5478d126afe3357045220d8be1ef9c6859ee8427f28984ff8d169b9ab1b75902R126

qcodes/instrument/parameter.py Show resolved Hide resolved
@jenshnielsen jenshnielsen force-pushed the feat/unit_is_a_property branch from befde0b to 1fe6b12 Compare February 16, 2022 13:49
@jenshnielsen jenshnielsen merged commit 9cc11fe into microsoft:master Feb 16, 2022
@jenshnielsen jenshnielsen deleted the feat/unit_is_a_property branch October 25, 2024 11:47
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.

2 participants