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

Rohde Schwarz ZNB /VNA refactor common code #887

Merged
merged 31 commits into from
Feb 7, 2018

Conversation

Dominik-Vogel
Copy link
Contributor

@Dominik-Vogel Dominik-Vogel commented Nov 28, 2017

In this pull request the common code from the two sweep classes gets refactored out.
The changes have not been tested with the instrument yet.

Additionally the channel class is abstracted into a class variable in the same way as in the decadac #876 to enable extention.
I also added a feature to automatically select the trace for the sweep when get is being called and restore the previous state on finalization.

@Dominik-Vogel
Copy link
Contributor Author

This way of subclassing a parameter and overriding a method does not support the functionality of get_latest(). So we would need to add a _save_val/supply the get function via a kwarg or implement the warpper manually

@Dominik-Vogel
Copy link
Contributor Author

Okay I jsut learned, that BaseParameter actually does the wrapping....

@jenshnielsen
Copy link
Collaborator

jenshnielsen commented Nov 30, 2017

@Dominik-Vogel Looks like a nice cleanup. I would very much like this to be tested on an actual instrument before merging. @ThorvaldLarsen are you using the vna and able to test this

@jenshnielsen
Copy link
Collaborator

You might want to rename get -> get_raw to get rid of the deprecation warning

@Dominik-Vogel
Copy link
Contributor Author

Thanks for the tip with get_raw! Yes surly do I want to test it before merging it.

@ThorvaldLarsen
Copy link
Contributor

I can help with testing tomorrow afternoon if it is ready for testing by then.

@Dominik-Vogel Dominik-Vogel mentioned this pull request Dec 6, 2017
3 tasks
@codecov
Copy link

codecov bot commented Dec 15, 2017

Codecov Report

Merging #887 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #887   +/-   ##
=======================================
  Coverage   77.92%   77.92%           
=======================================
  Files          37       37           
  Lines        5236     5236           
=======================================
  Hits         4080     4080           
  Misses       1156     1156

@@ -68,7 +68,7 @@ def __init__(self, name, instrument, start, stop, npts, channel):
label='{} magnitude'.format(
instrument._vna_parameter),
setpoint_units=('Hz',),
setpoint_names=('{}_frequency'.format(instrument._vna_parameter),))
Copy link
Contributor

@ThorvaldLarsen ThorvaldLarsen Jan 22, 2018

Choose a reason for hiding this comment

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

This is a mistake by me. Should be '{}_frequency' with the '_'.

# source power is dependent on model, but not well documented.
# here we assume -60 dBm for ZNB20, the others are set,
# due to lack of knowledge, to -80 dBm as of before the edit
model = self._instrument.get_idn()['model'].split('-')[0]
Copy link
Contributor

Choose a reason for hiding this comment

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

model = self._parent.get_idn()['model'].split('-')[0]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good point, thanks 👍

@ThorvaldLarsen
Copy link
Contributor

@Dominik-Vogel I just remembered a somewhat minor thing but now that we are redoing the driver we might as well add it.
The bandwidth can only be set in steps 1/1.5/2/3/5/7, i.e. 1kHz, 700Hz, 15Hz etc. That is 280Hz will be set to the nearest step in this case 300Hz.
Is it possible to make a parser that checks if the number is allowed and if not reports back the actual value being set on the VNA?

See page 4 in https://cdn.rohde-schwarz.com/pws/dl_downloads/dl_common_library/dl_brochures_and_datasheets/pdf_1/service_support_30/ZNB_dat-sw_en_5214-5384-22_v0900_96dp.pdf

@WilliamHPNielsen
Copy link
Contributor

@ThorvaldLarsen Sorry to jump in in the middle of a discussion, but I just caught a glimpse of something that sounds dangerous. Behaviour like

my_inst.my_param(100)  # actually sets it to 120

is in my opinion very bad and should be avoided. Either you input a correct value and have that exact value (modulo rounding errors and noise) realised on the instrument or you get an error and nothing happens on the insturment. As you most likely know, this is handled by validators.

@ThorvaldLarsen
Copy link
Contributor

ThorvaldLarsen commented Jan 24, 2018

@WilliamHPNielsen I have no clue what the correct way to handle this is in Qcodes. I am just pointing out that the behaviour of the ZNB instrument when setting bandwidth is pretty unintuitive. If you try to set the bandwidth to 120 Hz it will silently round it off to nearest step being 100 Hz.

Ahh, think I missread your comment at first. Using a validator to check if the number is allowed before talking to the instrument sounds good.

@WilliamHPNielsen
Copy link
Contributor

@ThorvaldLarsen I see. It is very good that you point this is out. Then the correct way is indeed to make any attempt of setting a value that can not be realised fail using an appropriate validator. We are paranoid about machines silently doing things behind our backs.

@WilliamHPNielsen
Copy link
Contributor

@ThorvaldLarsen If you need help writing a custom validator for this case, just ping me.

@Dominik-Vogel
Copy link
Contributor Author

@ThorvaldLarsen, @WilliamHPNielsen I'll add the validator.

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.

Left a few very minor comments

@@ -42,11 +21,12 @@ def __init__(self, name, instrument, start, stop, npts, channel):
self._channel = channel
self.names = ('magnitude',
'phase')
self.labels = ('{} magnitude'.format(instrument._vna_parameter),
'{} phase'.format(instrument._vna_parameter))
self.labels = ('{} magnitude'.format(instrument.short_name),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you remind me of name, full_name vs short name

Copy link
Contributor

Choose a reason for hiding this comment

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

full name is 'Instrument name'_'Channel name' and short_name is 'Channel name'.
In my case full name is VNA_S21 for and short_name is S21.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ok thats cool

data = self._instrument._get_sweep_data()
if self._instrument.format() in ['Polar', 'Complex',
'Smith', 'Inverse Smith']:
log.warning("QCoDeS Dataset does not currently support Complex "
Copy link
Collaborator

Choose a reason for hiding this comment

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

Perhaps this warning should point to the other parameter

Copy link
Contributor Author

@Dominik-Vogel Dominik-Vogel Feb 6, 2018

Choose a reason for hiding this comment

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

good point, I'll add it.

def __init__(self, parent, name, channel, vna_parameter: str=None):
"""
Args:
vna_parameter: if left empty name is used
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would write a bit more here.

Args:
    parent: Instrument that this channel is bound to.
    name: Name to use for this parameter
    vna_parameter: Name of parameter on the vna that this should measure such as S11. If left
               empty this will fall back to `name` 

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ThorvaldLarsen could you write this? I think you can find more concise words than me ;-)

Copy link
Contributor

@ThorvaldLarsen ThorvaldLarsen Feb 6, 2018

Choose a reason for hiding this comment

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

Might just want to change a word or two for clarity. But the part that Jens wrote looks pretty good to me.

Args:
    parent: Instrument that this channel is bound to.
    name: Name to use for this channel.
    vna_parameter: Name of parameter on the vna that this should measure such as S12. If left
               empty this will fall back to `name`.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done. Ready to merge?

@jenshnielsen jenshnielsen merged commit e4986ac into microsoft:master Feb 7, 2018
giulioungaretti pushed a commit that referenced this pull request Feb 7, 2018
Author: Dominik Vogel <[email protected]>

    Rohde Schwarz ZNB /VNA refactor common code (#887)
bors bot added a commit that referenced this pull request Dec 29, 2021
3746: Bump types-docutils from 0.17.1 to 0.17.2 r=jenshnielsen a=dependabot[bot]

Bumps [types-docutils](https:/python/typeshed) from 0.17.1 to 0.17.2.
<details>
<summary>Commits</summary>
<ul>
<li>See full diff in <a href="https:/python/typeshed/commits">compare view</a></li>
</ul>
</details>
<br />


[![Dependabot compatibility score](https://dependabot-badges.githubapp.com/badges/compatibility_score?dependency-name=types-docutils&package-manager=pip&previous-version=0.17.1&new-version=0.17.2)](https://docs.github.com/en/github/managing-security-vulnerabilities/about-dependabot-security-updates#about-compatibility-scores)

You can trigger a rebase of this PR by commenting ``@dependabot` rebase`.

[//]: # (dependabot-automerge-start)
[//]: # (dependabot-automerge-end)

---

<details>
<summary>Dependabot commands and options</summary>
<br />

You can trigger Dependabot actions by commenting on this PR:
- ``@dependabot` rebase` will rebase this PR
- ``@dependabot` recreate` will recreate this PR, overwriting any edits that have been made to it
- ``@dependabot` merge` will merge this PR after your CI passes on it
- ``@dependabot` squash and merge` will squash and merge this PR after your CI passes on it
- ``@dependabot` cancel merge` will cancel a previously requested merge and block automerging
- ``@dependabot` reopen` will reopen this PR if it is closed
- ``@dependabot` close` will close this PR and stop Dependabot recreating it. You can achieve the same result by closing it manually
- ``@dependabot` ignore this major version` will close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself)
- ``@dependabot` ignore this minor version` will close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself)
- ``@dependabot` ignore this dependency` will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself)


</details>

3747: Bump attrs from 21.2.0 to 21.3.0 r=jenshnielsen a=dependabot[bot]

Bumps [attrs](https:/python-attrs/attrs) from 21.2.0 to 21.3.0.
<details>
<summary>Release notes</summary>
<p><em>Sourced from <a href="https:/python-attrs/attrs/releases">attrs's releases</a>.</em></p>
<blockquote>
<h2>21.3.0</h2>
<p>This is a big release in the history of <code>attrs</code> and finishes an arc that took way too long and also delayed this very overdue release. But it's done: <code>import attrs</code> that has been talked about for years[^issue], but fell victim to “just this one more thing” has <em>finally</em> landed.</p>
<p>From now on, modern <code>attrs</code> code looks like this:</p>
<pre lang="python"><code>from attrs import define
<p><a href="https:/define"><code>`@​define</code></a>`
class HelloWorld:
modern: bool = True
</code></pre></p>
<p>The <code>define</code>/<code>field</code> APIs have been around for over a year and were very popular, now the rest of the package followed suit. I'm very excited that <code>attrs</code> remains relevant and keeps evolving over now more than half a decade. If you're curious about some of the background, the docs now contain a short <a href="https://www.attrs.org/en/stable/names.html">explanation and history lesson</a>. As long as our users keep pushing us, we will keep pushing forward class generation in Python!</p>
<p>Big thanks to my <a href="https:/sponsors/hynek/dashboard">GitHub Sponsors</a>, <a href="https://tidelift.com/subscription/pkg/pypi-attrs?utm_source=pypi-attrs&amp;utm_medium=referral&amp;utm_campaign=enterprise&amp;utm_term=repo">Tidelift subscribers</a>, and <a href="https://ko-fi.com/the_hynekl">Ko-fi buyers</a> that help me mustering the motivation for such long-running project!</p>
<hr />
<p>Since the release took so long, there's more highlights than we can enumerate here, we'd just like to point out a breaking change in the new APIs: converters now run on setting attributes by default. If this is causing problems to you, you can disable that behavior by setting <code>`@define(on_setattr=[])</code>.</p>`
<p>[^issue]:  I have an issue from 2018 that I wanted to &quot;come back the moment this lands&quot;.</p>
<h1>Full Changelog</h1>
<h2>Backward-incompatible Changes</h2>
<ul>
<li>
<p>When using <code>`@define</code>,` converters are now run by default when setting an attribute on an instance -- additionally to validators. I.e. the new default is <code>on_setattr=[attrs.setters.convert, attrs.setters.validate]</code>.</p>
<p>This is unfortunately a breaking change, but it was an oversight, impossible to raise a <code>DeprecationWarning</code> about, and it's better to fix it now while the APIs are very fresh with few users. <a href="https://github-redirect.dependabot.com/python-attrs/attrs/issues/835">#835</a>, <a href="https://github-redirect.dependabot.com/python-attrs/attrs/issues/886">#886</a></p>
</li>
<li>
<p><code>import attrs</code> has finally landed! As of this release, you can finally import <code>attrs</code> using its proper name.</p>
<p>Not all names from the <code>attr</code> namespace have been transferred; most notably <code>attr.s</code> and <code>attr.ib</code> are missing. See <code>attrs.define</code> and <code>attrs.field</code> if you haven't seen our next-generation APIs yet. A more elaborate explanation can be found <a href="https://www.attrs.org/en/latest/names.html">On The Core API Names</a></p>
<p>This feature is at least for one release <strong>provisional</strong>. We don't <em>plan</em> on changing anything, but such a big change is unlikely to go perfectly on the first strike.</p>
<p>The API docs have been mostly updated, but it will be an ongoing effort to change everything to the new APIs. Please note that we have <strong>not</strong> moved -- or even removed -- anything from <code>attr</code>!</p>
<p>Please do report any bugs or documentation inconsistencies! <a href="https://github-redirect.dependabot.com/python-attrs/attrs/issues/887">#887</a></p>
</li>
</ul>
<h2>Changes</h2>
<ul>
<li><code>attr.asdict(retain_collection_types=False)</code> (default) dumps collection-esque keys as tuples. <a href="https://github-redirect.dependabot.com/python-attrs/attrs/issues/646">#646</a>, <a href="https://github-redirect.dependabot.com/python-attrs/attrs/issues/888">#888</a></li>
<li><code>__match_args__</code> are now generated to support Python 3.10's <a href="https://docs.python.org/3.10/whatsnew/3.10.html#pep-634-structural-pattern-matching">Structural Pattern Matching</a>. This can be controlled by the <code>match_args</code> argument to the class decorators on Python 3.10 and later. On older versions, it is never added and the argument is ignored. <a href="https://github-redirect.dependabot.com/python-attrs/attrs/issues/815">#815</a></li>
<li>If the class-level <em>on_setattr</em> is set to <code>attrs.setters.validate</code> (default in <code>`@define</code>` and <code>`@mutable</code>)` but no field defines a validator, pretend that it's not set. <a href="https://github-redirect.dependabot.com/python-attrs/attrs/issues/817">#817</a></li>
<li>The generated <code>__repr__</code> is significantly faster on Pythons with f-strings. <a href="https://github-redirect.dependabot.com/python-attrs/attrs/issues/819">#819</a></li>
<li>Attributes transformed via <code>field_transformer</code> are wrapped with <code>AttrsClass</code> again. <a href="https://github-redirect.dependabot.com/python-attrs/attrs/issues/824">#824</a></li>
<li>Generated source code is now cached more efficiently for identical classes. <a href="https://github-redirect.dependabot.com/python-attrs/attrs/issues/828">#828</a></li>
<li>Added <code>attrs.converters.to_bool()</code>. <a href="https://github-redirect.dependabot.com/python-attrs/attrs/issues/830">#830</a></li>
</ul>
<!-- raw HTML omitted -->
</blockquote>
<p>... (truncated)</p>
</details>
<details>
<summary>Changelog</summary>
<p><em>Sourced from <a href="https:/python-attrs/attrs/blob/main/CHANGELOG.rst">attrs's changelog</a>.</em></p>
<blockquote>
<h2>21.3.0 (2021-12-28)</h2>
<p>Backward-incompatible Changes
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^</p>
<ul>
<li>
<p>When using <code>`@define</code>,` converters are now run by default when setting an attribute on an instance -- additionally to validators.
I.e. the new default is <code>on_setattr=[attrs.setters.convert, attrs.setters.validate]</code>.</p>
<p>This is unfortunately a breaking change, but it was an oversight, impossible to raise a <code>DeprecationWarning</code> about, and it's better to fix it now while the APIs are very fresh with few users.
<code>[#835](python-attrs/attrs#835) &lt;https:/python-attrs/attrs/issues/835&gt;</code><em>,
<code>[#886](python-attrs/attrs#886) &lt;https:/python-attrs/attrs/issues/886&gt;</code></em></p>
</li>
<li>
<p><code>import attrs</code> has finally landed!
As of this release, you can finally import <code>attrs</code> using its proper name.</p>
<p>Not all names from the <code>attr</code> namespace have been transferred; most notably <code>attr.s</code> and <code>attr.ib</code> are missing.
See <code>attrs.define</code> and <code>attrs.field</code> if you haven't seen our next-generation APIs yet.
A more elaborate explanation can be found <code>On The Core API Names &lt;https://www.attrs.org/en/latest/names.html&gt;</code>_</p>
<p>This feature is at least for one release <strong>provisional</strong>.
We don't <em>plan</em> on changing anything, but such a big change is unlikely to go perfectly on the first strike.</p>
<p>The API docs have been mostly updated, but it will be an ongoing effort to change everything to the new APIs.
Please note that we have <strong>not</strong> moved -- or even removed -- anything from <code>attr</code>!</p>
<p>Please do report any bugs or documentation inconsistencies!
<code>[#887](python-attrs/attrs#887) &lt;https:/python-attrs/attrs/issues/887&gt;</code>_</p>
</li>
</ul>
<p>Changes
^^^^^^^</p>
<ul>
<li><code>attr.asdict(retain_collection_types=False)</code> (default) dumps collection-esque keys as tuples.
<code>[#646](python-attrs/attrs#646) &lt;https:/python-attrs/attrs/issues/646&gt;</code><em>,
<code>[#888](python-attrs/attrs#888) &lt;https:/python-attrs/attrs/issues/888&gt;</code></em></li>
<li><code>__match_args__</code> are now generated to support Python 3.10's
<code>Structural Pattern Matching &lt;https://docs.python.org/3.10/whatsnew/3.10.html#pep-634-structural-pattern-matching&gt;</code><em>.
This can be controlled by the <code>match_args</code> argument to the class decorators on Python 3.10 and later.
On older versions, it is never added and the argument is ignored.
<code>[#815](python-attrs/attrs#815) &lt;https:/python-attrs/attrs/issues/815&gt;</code></em></li>
<li>If the class-level <em>on_setattr</em> is set to <code>attrs.setters.validate</code> (default in <code>`@define</code>` and <code>`@mutable</code>)` but no field defines a validator, pretend that it's not set.
<code>[#817](python-attrs/attrs#817) &lt;https:/python-attrs/attrs/issues/817&gt;</code>_</li>
<li>The generated <code>__repr__</code> is significantly faster on Pythons with f-strings.
<code>[#819](python-attrs/attrs#819) &lt;https:/python-attrs/attrs/issues/819&gt;</code>_</li>
<li>Attributes transformed via <code>field_transformer</code> are wrapped with <code>AttrsClass</code> again.
<code>[#824](python-attrs/attrs#824) &lt;https:/python-attrs/attrs/issues/824&gt;</code>_</li>
<li>Generated source code is now cached more efficiently for identical classes.
<code>[#828](python-attrs/attrs#828) &lt;https:/python-attrs/attrs/issues/828&gt;</code>_</li>
<li>Added <code>attrs.converters.to_bool()</code>.
<code>[#830](python-attrs/attrs#830) &lt;https:/python-attrs/attrs/issues/830&gt;</code>_</li>
</ul>
<!-- raw HTML omitted -->
</blockquote>
<p>... (truncated)</p>
</details>
<details>
<summary>Commits</summary>
<ul>
<li><a href="https:/python-attrs/attrs/commit/dd26edd68e12879f716c6554f25d957af299b801"><code>dd26edd</code></a> Prepare 21.3.0</li>
<li><a href="https:/python-attrs/attrs/commit/20bf4b6e54a75201b378cff8e6dd9521d2da28f1"><code>20bf4b6</code></a> Go over CONTRIBUTING.md</li>
<li><a href="https:/python-attrs/attrs/commit/d528dd425980eff3f43b0e29b0ce4dc81ecd8d84"><code>d528dd4</code></a> Fix more links</li>
<li><a href="https:/python-attrs/attrs/commit/fcfb5a692cc8c9f8fde8e39bbd2c5733a47fb1e7"><code>fcfb5a6</code></a> Last pass over changelogs</li>
<li><a href="https:/python-attrs/attrs/commit/e09873485e14e9b11d5d590a55280894df367d92"><code>e098734</code></a> Add logo to PyPI description</li>
<li><a href="https:/python-attrs/attrs/commit/26c0cef8e48bd131d062d45bdaa0c949d4a2d035"><code>26c0cef</code></a> Streamline workflow</li>
<li><a href="https:/python-attrs/attrs/commit/3333e749781a107c829717f7bc0382d33b538b6e"><code>3333e74</code></a> Remove dead achor</li>
<li><a href="https:/python-attrs/attrs/commit/2d77d83d4e3ceadbf4414da5963623a20564c415"><code>2d77d83</code></a> Fix dataclass_transform links</li>
<li><a href="https:/python-attrs/attrs/commit/b4dc9b07c70c16848960da077fc7ac18fe5e9bc8"><code>b4dc9b0</code></a> Better 2.7 example</li>
<li><a href="https:/python-attrs/attrs/commit/d4e32209dc5855796e57c2b08bdc1c1702d051ab"><code>d4e3220</code></a> Use attrs namespace throughout examples.rst</li>
<li>Additional commits viewable in <a href="https:/python-attrs/attrs/compare/21.2.0...21.3.0">compare view</a></li>
</ul>
</details>
<br />


[![Dependabot compatibility score](https://dependabot-badges.githubapp.com/badges/compatibility_score?dependency-name=attrs&package-manager=pip&previous-version=21.2.0&new-version=21.3.0)](https://docs.github.com/en/github/managing-security-vulnerabilities/about-dependabot-security-updates#about-compatibility-scores)

You can trigger a rebase of this PR by commenting ``@dependabot` rebase`.

[//]: # (dependabot-automerge-start)
[//]: # (dependabot-automerge-end)

---

<details>
<summary>Dependabot commands and options</summary>
<br />

You can trigger Dependabot actions by commenting on this PR:
- ``@dependabot` rebase` will rebase this PR
- ``@dependabot` recreate` will recreate this PR, overwriting any edits that have been made to it
- ``@dependabot` merge` will merge this PR after your CI passes on it
- ``@dependabot` squash and merge` will squash and merge this PR after your CI passes on it
- ``@dependabot` cancel merge` will cancel a previously requested merge and block automerging
- ``@dependabot` reopen` will reopen this PR if it is closed
- ``@dependabot` close` will close this PR and stop Dependabot recreating it. You can achieve the same result by closing it manually
- ``@dependabot` ignore this major version` will close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself)
- ``@dependabot` ignore this minor version` will close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself)
- ``@dependabot` ignore this dependency` will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself)


</details>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
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.

5 participants