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

Allow getting an uninitialized DelegateParameter with offset or scale #3662

Conversation

haroldmeerwaldt
Copy link
Contributor

@haroldmeerwaldt haroldmeerwaldt commented Dec 10, 2021

  • do not apply offset or scale when getting delegate parameter and source is None (in _from_raw_value_to_value)
  • added two tests

Fixes #3653

@haroldmeerwaldt haroldmeerwaldt changed the title Allow getting an uninitialized DelegateParameter with scale Allow getting an uninitialized DelegateParameter with offset or scale Dec 10, 2021
@haroldmeerwaldt
Copy link
Contributor Author

@astafan8 I put the tests in test_parameter_scale_offset.py as you asked. A good place would also be test_delegate_parameter.py.

As I was scrolling through it, I found that the previous behaviour may actually be intended:
https://github.com/QCoDeS/Qcodes/blob/c4fa2c8f83854ff989ca381e9bc65cfd9eac067b/qcodes/tests/parameter/test_delegate_parameter.py#L253

What do you think about the other location, and the change of expected behaviour?

@astafan8
Copy link
Contributor

A good place would also be test_delegate_parameter.py.

true, but i don't think that matters much. leave this decision up to your preference :)

As I was scrolling through it, I found that the previous behaviour may actually be intended:
Qcodes/qcodes/tests/parameter/test_delegate_parameter.py. What do you think about the change of expected behaviour?

hmmm..... that's a very good question! glad that we have these tests so that we think about this. However, i think this test is only capturing the current behavior as opposed to making it "desired". I don't think it makes sense that get() and snapshot() rasie a TypeError with the scale/offset set and ready while the underlying parameter's value is None - that's where the original issue is coming from, right? that this behavior does not bring value.

we can ask for a third opinion - @jenshnielsen what is your take on this?

@jenshnielsen
Copy link
Collaborator

I think its fine to change the test to reflect this new behavior. Just make sure that we document this by adding a changelog fragment

@codecov
Copy link

codecov bot commented Dec 13, 2021

Codecov Report

Merging #3662 (74d2eb7) into master (c4fa2c8) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #3662   +/-   ##
=======================================
  Coverage   65.74%   65.74%           
=======================================
  Files         225      225           
  Lines       30524    30524           
=======================================
  Hits        20068    20068           
  Misses      10456    10456           

@jenshnielsen
Copy link
Collaborator

bors merge

@bors bors bot merged commit 7e2e059 into microsoft:master Dec 14, 2021
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.

Getting a DelegateParameter with scale fails if it is not initialized
3 participants