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

Use the full names when registering multiparameters #2317

Merged
merged 4 commits into from
Oct 23, 2020

Conversation

jenshnielsen
Copy link
Collaborator

@jenshnielsen jenshnielsen commented Oct 20, 2020

For some reason multiparameters were unlike all other paramters not using the full name. This is both confusing and problematic if you have two multiparameters with the same short name (e.g. from different channels)

@astafan8 Any suggestions for other ways of doing it?

Note This is a breaking change but I don't see any way around it.

@codecov
Copy link

codecov bot commented Oct 20, 2020

Codecov Report

Merging #2317 into master will decrease coverage by 0.00%.
The diff coverage is 83.33%.

@@            Coverage Diff             @@
##           master    #2317      +/-   ##
==========================================
- Coverage   61.87%   61.86%   -0.01%     
==========================================
  Files         201      201              
  Lines       26122    26119       -3     
==========================================
- Hits        16162    16159       -3     
  Misses       9960     9960              

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.

@astafan8 Any suggestions for other ways of doing it?

no, becasue it think this IS the correct way of doing it.

Note This is a breaking change but I don't see any way around it.

me neither. i'd say this is a discovery of a bug, which is great.


Another way of fixing this PR could be to change __str__ to just return self.full_name as opposed to the current implementation here https://github.com/QCoDeS/Qcodes/blob/master/qcodes/instrument/parameter.py#L387 . I actually think that changing __str__ must also be done regardless of this PR :)

@jenshnielsen
Copy link
Collaborator Author

The changes to not use str(parameter) are actually only for consistency. The real issue is the fix
from self._register_parameter(multiparameter.names[i], to self._register_parameter(multiparameter.full_names[i],

@astafan8
Copy link
Contributor

The changes to not use str(parameter) are actually only for consistency

that's fine, it's just that when reviewing i had to look and see what str(parameter) does to confirm the consistency ;)

@jenshnielsen jenshnielsen force-pushed the fix_multiparam_names branch 2 times, most recently from a5324af to 9e92968 Compare October 22, 2020 11:19
@github-actions github-actions bot merged commit a542d28 into microsoft:master Oct 23, 2020
@jenshnielsen jenshnielsen deleted the fix_multiparam_names branch October 25, 2024 11:50
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