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

val_mapping creator function to solve on/off/true/false issue #1413

Merged
merged 24 commits into from
Feb 15, 2019

Conversation

nataliejpg
Copy link
Contributor

@nataliejpg nataliejpg commented Dec 5, 2018

Fixes #924

I made a function which given the instrument values for 'on' and 'off' maps all the obvious inputs (1, True, 'On', 'on', 'ON') to on and off. The inverse always maps back to booleans which I chose relatively arbitrarily as 'the most sensible' to be the convention. Happy to extend it if more are voted for.

Also, Implemented in the rhode schwarz SGS100A for now but I imagine other drivers would also benefit from it.

TODO:

  • Test
  • Add a note about it to driver creating notebook doc
  • Use in other drivers?

@WilliamHPNielsen @jenshnielsen

@codecov
Copy link

codecov bot commented Dec 10, 2018

Codecov Report

Merging #1413 into master will increase coverage by 0.02%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #1413      +/-   ##
==========================================
+ Coverage   73.86%   73.88%   +0.02%     
==========================================
  Files          92       92              
  Lines       10411    10420       +9     
==========================================
+ Hits         7690     7699       +9     
  Misses       2721     2721

qcodes/utils/helpers.py Outdated Show resolved Hide resolved
@astafan8
Copy link
Contributor

I've refactored the code, and added tests.

Questions:

  • What other "sensible" on/off inputs we need?
  • Do we also need enabled/disabled val mapping? Or shall that case be strongly recommended to be True/False and parameter should always be called smth_enabled so that the API is smth_enabled(True)?

@jenshnielsen
Copy link
Collaborator

  • I think this is fine
  • I vote for leaving that out for now

@astafan8 astafan8 changed the title [WIP] val_mapping to solve on/off/true/false issue val_mapping creator function to solve on/off/true/false issue Feb 14, 2019
@astafan8
Copy link
Contributor

@QCoDeS/core ready for review.

Question: shall i also go through all drivers quickly and use this new function in cases where it makes sense and does not break instrument-driver's API?

@astafan8
Copy link
Contributor

@nataliejpg as the original author of the PR and the main feature requester, could you also have a look? :)

@astafan8 astafan8 self-assigned this Feb 14, 2019
@nataliejpg
Copy link
Contributor Author

looks good to me @astafan8, thanks for the work. I didn't understand your question:

Do we also need enabled/disabled val mapping? Or shall that case be strongly recommended to be True/False and parameter should always be called smth_enabled so that the API is smth_enabled(True)?

@astafan8
Copy link
Contributor

@nataliejpg

I didn't understand your question:

Do we also need enabled/disabled val mapping? Or shall that case be strongly recommended to be True/False and parameter should always be called smth_enabled so that the API is smth_enabled(True)?

I also don't really remember :( I think it was about parameters which have enabled/disabled meaning which might probably be different from on/off meaning in some cases (definitely not in this PR's rhode schwarz driver), if we need a create_en_dis_abled_val_mapping function of some sort.

But now i think we don't, and also this question of mine is a bit "stupid" :)

Copy link
Contributor

@Dominik-Vogel Dominik-Vogel left a comment

Choose a reason for hiding this comment

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

I this PR is sensible, clean, and good to go

@astafan8 astafan8 merged commit df010d1 into microsoft:master Feb 15, 2019
giulioungaretti pushed a commit that referenced this pull request Feb 15, 2019
Merge: 4392f24 48f3302
Author: Mikhail Astafev <astafan8@gmail.com>

    Merge pull request #1413 from nataliejpg/on_off_val_mapping
@nataliejpg nataliejpg deleted the on_off_val_mapping branch April 15, 2019 09:45
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.

None yet

4 participants