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.

Sign up for GitHub

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

Jump to bottom

Fixes and improvements to N52xx PNA Driver #3688

Merged
merged 12 commits into from
Dec 20, 2021
Merged

Fixes and improvements to N52xx PNA Driver #3688

merged 12 commits into from
Dec 20, 2021

Conversation

spauka
Copy link
Contributor

@spauka spauka commented Dec 15, 2021 •
edited
Loading

Changes proposed in this PR:

  • Fix warnings on instrument initialization caused by **kwargs not being forwarded by PNASweep (closes N52xx PNA Driver Warnings on Initialization #3687)
  • Use new instrument logger
  • Update old ArrayParameters to use ParameterWithSetpoints instead
  • Correctly label axis labels when sweeping in CW mode or with a LOG frequency sweep

Note: This PR depends on #3683. I will rebase it once that changeset is merged.

@jenshnielsen

@astafan8 astafan8 mentioned this pull request Dec 15, 2021
Merged
astafan8
astafan8 approved these changes Dec 15, 2021
View reviewed changes
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.

thank you @spauka !

docs/changes/newsfragments/3688.n52xx Outdated Show resolved Hide resolved
@astafan8 astafan8 added this to the 0.31.0 milestone Dec 15, 2021
@spauka spauka force-pushed the feat/fix_n52xx_base_warnings branch 2 times, most recently from b22d000 to f1dc3f3 Compare December 16, 2021 05:32
@spauka spauka marked this pull request as ready for review December 16, 2021 05:35
@spauka
Copy link
Contributor Author

spauka commented Dec 16, 2021

Since 3683 is now merged I've rebased this PR and it should be ready to merge.

I've tested it with the N5245A network analyzer and it works correctly.

@codecov
Copy link

codecov bot commented Dec 16, 2021 •
edited
Loading

Codecov Report

Merging #3688 (3651da6) into master (5b2aec2) will decrease coverage by 0.03%.
The diff coverage is 38.77%.

@@            Coverage Diff             @@
##           master    #3688      +/-   ##
==========================================
- Coverage   65.73%   65.70%   -0.04%     
==========================================
  Files         226      226              
  Lines       30547    30561      +14     
==========================================
  Hits        20080    20080              
- Misses      10467    10481      +14

@spauka
Copy link
Contributor Author

spauka commented Dec 16, 2021

@astafan8 I'm not sure I understand why the docs are failing to build?

@spauka spauka force-pushed the feat/fix_n52xx_base_warnings branch from 5803b4f to 82e25b1 Compare December 16, 2021 11:10
@jenshnielsen
Copy link
Collaborator

@spauka In general you don't need to worry about adding prama comments to disable codacy warnings. We look at codacy as guidelines not as requirements, which is also why the job is not marked as required to pass

@spauka
Copy link
Contributor Author

spauka commented Dec 16, 2021

Ah, thanks. I really need to re-read the contributions guide, I just noticed it was there too 😆

Would you like me to remove the codacy comments? The mypy ones I think have to stay otherwise the CI tests fail...

@jenshnielsen
Copy link
Collaborator

@spauka I don't think they are harmful so feel free to leave them. The mypy ones should stay. For bonus points you can annotate the mypy ones with error type like here https://github.com/QCoDeS/Qcodes/blob/master/qcodes/instrument/channel.py#L880
The easiest way to get these error codes is to run mypy locally with --show-error-code and without the ignore comments

@spauka spauka force-pushed the feat/fix_n52xx_base_warnings branch from cc25357 to 1cc0bf1 Compare December 19, 2021 23:22
astafan8
astafan8 approved these changes Dec 20, 2021
View reviewed changes
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.

bors merge

@bors
Copy link
Contributor

bors bot commented Dec 20, 2021

Canceled.

@astafan8
Copy link
Contributor

bors merge

@bors
Copy link
Contributor

bors bot commented Dec 20, 2021

🕐 Waiting for PR status (Github check) to be set, probably by CI. Bors will automatically try to run when all required PR statuses are set.

@astafan8
Copy link
Contributor

bors merge

@bors
Copy link
Contributor

bors bot commented Dec 20, 2021

Build succeeded:

@bors bors bot merged commit 681dd67 into microsoft:master Dec 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jenshnielsen jenshnielsen jenshnielsen left review comments

@astafan8 astafan8 astafan8 approved these changes

Assignees
No one assigned
Labels
driver
Projects
None yet
Milestone
0.31.0
Development

Successfully merging this pull request may close these issues.

N52xx PNA Driver Warnings on Initialization
3 participants
@spauka @jenshnielsen @astafan8