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

docs: Fix arrays in data set #310

Merged
merged 1 commit into from
Aug 24, 2016
Merged

docs: Fix arrays in data set #310

merged 1 commit into from
Aug 24, 2016

Conversation

giulioungaretti
Copy link
Contributor

See QCoDeS/Qcodes_loop#20 .

Changes proposed in this pull request:

  • Fix the docs

@alexcjohnson say the word, as that's your code.

It seems the attribute and the arguments where mixed up!

It seems the attribute and the arguments where mixed up!
@qcodes-bot
Copy link

qcodes-bot commented Aug 21, 2016

Current coverage is 82.82% (diff: 100%)

Merging #310 into master will not change coverage

@@             master       QCoDeS/Qcodes#310   diff @@
==========================================
  Files            34         34          
  Lines          3615       3615          
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
  Hits           2994       2994          
  Misses          621        621          
  Partials          0          0          

Powered by Codecov. Last update 934b83d...1e8f9b9

@alexcjohnson
Copy link
Contributor

💃
Right, the constructor arg and the corresponding (resulting) attribute do not match. Dunno if this is something we want to change in the future so they DO match, that would be nice but:

  • as a constructor arg, a dict would be redundant because you need to map {array_id:array} when array_id is already specified inside each array
  • but as an attribute, we need to be able to efficiently look up arrays by their ids
    So I'd vote to just make it clear that the arg and the attribute are different.

@giulioungaretti
Copy link
Contributor Author

I think it's fine like this with the docs fixed.

@giulioungaretti giulioungaretti merged commit da80aa0 into microsoft:master Aug 24, 2016
giulioungaretti pushed a commit that referenced this pull request Aug 24, 2016
Merge: 934b83d 1e8f9b9
Author: Giulio Ungaretti <[email protected]>

    Merge pull request #310 from giulioungaretti/master
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.

3 participants