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

_BaseParameter cached value: add set_cache method, make _save_val method useless, convert raw_value attribute to property #1757

Merged
merged 27 commits into from
Oct 25, 2019

Conversation

astafan8
Copy link
Contributor

@astafan8 astafan8 commented Oct 9, 2019

Description

This is needed for setting the cached value of a parameter without calling the meat of it's set method (for example, without communicating to the instrument, in case the parameter is part of an instrument).

Use cases that this covers for sure:

  • when initializing an instance of an instrument driver, this can be used to set the cached value of a parameter to a known default of an instrument for cases where instrument does not support querying that value, and only supports setting it to smth (good example, Keysight B1500 and its modules).
  • resolves Feature request: set_latest #1222 , for DynaCool driver

ToDo/inventory:

  • avoid code duplication of the wrap_set method in set_cached by refactoring -_from_value_to_raw_valueinto its own method and use both places (also refactor_from_raw_value_to_valuefromwrap_get`, just for fun)
  • testing, testing, testing
  • revise the name of the method - make it set_cache from set_latest
  • add _update_cache_from method that is the only one that updates the _latest dictionary, and use it in wrap_get
  • make necessary changes to DelegateParameter - a bug discovered where delegate.get_latest() does not forward the call to source.get_latest() which is a bug, issue Bug in DelegateParameter: get_latest and set_cache are not forwarded to source parameter #1782 created - fixing will happen in a separate PR.
  • make necessary changes to ParameterWithSetpoints to support it - the're none, so nothing to do
  • add docstrings
  • mention somewhere in the documentation won't do in this PR since get_latest is also not mentioned anywhere
  • link to github issues that this PR resolves
  • don't use _save_val in qcodes infra, use set_cache or _update_cache_with instead; _save_val deprecation will follow in other PRs
  • convert raw_value attribute to a property: on get it returns self._latest['raw_value'] and on set it only sets self._latest['raw_value']; deprecation, forbidding setting raw_value via the property, etc will follow in other PRs
  • don't use raw_value property in parameter.py

Related tasks for other PRs:

Its logic mimic that of the "set" method. Tests are trying to cover
all of the cases wrt scale/offset/validation/etc what the "set" method
seems to support.
@codecov
Copy link

codecov bot commented Oct 9, 2019

Codecov Report

Merging #1757 into master will increase coverage by 0.04%.
The diff coverage is 97.18%.

@@            Coverage Diff             @@
##           master    #1757      +/-   ##
==========================================
+ Coverage   67.44%   67.48%   +0.04%     
==========================================
  Files         147      147              
  Lines       18487    18511      +24     
==========================================
+ Hits        12468    12493      +25     
+ Misses       6019     6018       -1

@astafan8 astafan8 changed the title Add set_latest method to _BaseParameter Add set_cached method to _BaseParameter Oct 20, 2019
@astafan8 astafan8 marked this pull request as ready for review October 20, 2019 12:34
@astafan8
Copy link
Contributor Author

@QCoDeS/core Let's start reviewing of this PR. Please have a look at the tests in order to find out if the captured behavior is the one that we want/like. Please also let know if ideologically/conceptually DelegateParameter and ParameterWithSetpointshave to implement something special with respect toset_cached` method (or if it's good as-is).

Copy link
Collaborator

@jenshnielsen jenshnielsen left a comment

Choose a reason for hiding this comment

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

thanks, IMHO it's a bug in delegate parameter that get_latest on a delegate parameter does not always call get_latest on the parent parameter. This bug is made more obvious by this feature.

At a first glance I don't think there are any changes needed to parameterwithsetpoints

qcodes/instrument/parameter.py Outdated Show resolved Hide resolved
qcodes/instrument/parameter.py Show resolved Hide resolved
qcodes/tests/test_parameter.py Show resolved Hide resolved
becuase it's a generated method, which sphinx can't discover
@astafan8 astafan8 changed the title Add set_cached method to _BaseParameter _BaseParameter cached values: add set_cache[_from_raw] methods, deprecate raw_value attribute and _save_val method Oct 21, 2019
@astafan8 astafan8 changed the title _BaseParameter cached values: add set_cache[_from_raw] methods, deprecate raw_value attribute and _save_val method _BaseParameter cached value: add set_cache[_from_raw] methods, deprecate raw_value attribute and _save_val method Oct 21, 2019
@astafan8 astafan8 changed the title _BaseParameter cached value: add set_cache[_from_raw] methods, deprecate raw_value attribute and _save_val method _BaseParameter cached value: add set_cache and set_cache_from_raw methods, deprecate raw_value attribute and _save_val method Oct 21, 2019
because it just sets the raw value of cache, it doesn't do anything
extra that "from" might imply
@astafan8 astafan8 changed the title _BaseParameter cached value: add set_cache and set_cache_from_raw methods, deprecate raw_value attribute and _save_val method _BaseParameter cached value: add set_cache method, make _save_val method useless, convert raw_value attribute to property Oct 23, 2019
@astafan8
Copy link
Contributor Author

@QCoDeS/core ready for review again - see the description for the scope and future todo (i'll either make PRs for them shortly after this merge or add cards to our board).

... and not partial of the _save_val method
Copy link
Collaborator

@jenshnielsen jenshnielsen left a comment

Choose a reason for hiding this comment

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

Only small comments for the code. Will have a look at the tests later

qcodes/instrument/parameter.py Show resolved Hide resolved
qcodes/instrument/parameter.py Show resolved Hide resolved
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.

Feature request: set_latest
2 participants