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

Don't snapshot array and multiparameters per default #800

Merged

Conversation

jenshnielsen
Copy link
Collaborator

This is an alternative to #797 works by disabling snapshot storage for Array and Multiparamters by default

Pro this

  • we retain the wrappers including delays etc for all parameters:

Against

  • This is an still an api break. (Array/Multi)Parameters that used to do explicit _saveval will no longer be snapshotted unless they explicitly pass snapshot_value=True to the super class constructor, I have verified that this is the case for all current QCoDeS drivers.
  • If you don't want the wrappers in any _BaseParameter subclass you will have to replace them in your subclass with a noop

@WilliamHPNielsen @nulinspiratie

This is an API Change but mostly restores the behaviour \n prior to the merge of the parameter improvements
As we default to skipping array and multiparameters
@jenshnielsen jenshnielsen force-pushed the fix_param_snapshot_alt branch from 0ee1d99 to 6510a08 Compare October 19, 2017 11:10
@jenshnielsen jenshnielsen changed the title Don't snapshot array and multiparamters per default Don't snapshot array and multiparameters per default Oct 19, 2017
@WilliamHPNielsen
Copy link
Contributor

Re the two against points:

  • It seems unlikely that anyone is actually using ArrayParameter and MultiParameter out in the wild. If an API break seems unlikely to be bad, then... (I know, right)
  • This is perhaps a bit unfortunate, but it is currently unclear whether that is a likely scenario.

So... let's just go with this one.

@WilliamHPNielsen
Copy link
Contributor

WilliamHPNielsen commented Oct 20, 2017

[erroneously posted rubbish here]

Copy link
Contributor

@WilliamHPNielsen WilliamHPNielsen left a comment

Choose a reason for hiding this comment

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

Let's get this in.

@WilliamHPNielsen WilliamHPNielsen merged commit 6dccded into microsoft:master Oct 20, 2017
giulioungaretti pushed a commit that referenced this pull request Oct 20, 2017
Author: Jens Hedegaard Nielsen <jenshnielsen@gmail.com>

    Don't snapshot array and multiparameters per default (#800)
@jenshnielsen jenshnielsen deleted the fix_param_snapshot_alt branch October 20, 2017 13:43
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