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

Snapshot diff and method chaining #1363

Merged
merged 21 commits into from
Nov 7, 2018

Conversation

cgranade
Copy link
Contributor

@cgranade cgranade commented Nov 1, 2018

Changes proposed in this pull request:

  • Add new utility functions to quickly compare which parameter settings changed between two runs.
  • Make Measurement methods follow fluent style to allow for method chaining.

As per discussions with @astafan8 and @WilliamHPNielsen.

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.

Thanks for this!

Could you add some tests for both the changes?

qcodes/utils/metadata.py Outdated Show resolved Hide resolved
qcodes/utils/metadata.py Outdated Show resolved Hide resolved
qcodes/utils/metadata.py Show resolved Hide resolved
qcodes/utils/metadata.py Outdated Show resolved Hide resolved
qcodes/utils/metadata.py Outdated Show resolved Hide resolved
qcodes/utils/metadata.py Outdated Show resolved Hide resolved
@cgranade
Copy link
Contributor Author

cgranade commented Nov 1, 2018

Looks like I've got a few Mypy issues to fix as well. Let me update the PR with those once I figure it out.

@cgranade
Copy link
Contributor Author

cgranade commented Nov 6, 2018

I have no idea how #1375 got mixed in with this one, sorry. Let me try to fix that.

@codecov
Copy link

codecov bot commented Nov 7, 2018

Codecov Report

Merging #1363 into master will increase coverage by 0.07%.
The diff coverage is 94.73%.

@@            Coverage Diff             @@
##           master    #1363      +/-   ##
==========================================
+ Coverage   73.15%   73.22%   +0.07%     
==========================================
  Files          79       79              
  Lines        9156     9188      +32     
==========================================
+ Hits         6698     6728      +30     
- Misses       2458     2460       +2

@cgranade
Copy link
Contributor Author

cgranade commented Nov 7, 2018

@astafan8, would you be willing to take another look? Thanks!

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.

See my minor comments. Once you respond to them, this is ready to be merged.

Thanks!!!

qcodes/dataset/measurements.py Show resolved Hide resolved
qcodes/utils/metadata.py Outdated Show resolved Hide resolved
qcodes/utils/metadata.py Outdated Show resolved Hide resolved
qcodes/utils/metadata.py Outdated Show resolved Hide resolved
@cgranade
Copy link
Contributor Author

cgranade commented Nov 7, 2018

Thanks for the review, @astafan8!

@astafan8
Copy link
Contributor

astafan8 commented Nov 7, 2018

Thanks for the example notebook! once CI passes, i will merge.

@astafan8 astafan8 merged commit 284f2a2 into microsoft:master Nov 7, 2018
giulioungaretti pushed a commit that referenced this pull request Nov 7, 2018
Merge: 2e935d5 0c1aa52
Author: Mikhail Astafev <astafan8@gmail.com>

    Merge pull request #1363 from cgranade/cgranade/snapshot-diff
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants