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

Hotfix/live plot #270

Merged
merged 6 commits into from
Jul 21, 2016
Merged

Hotfix/live plot #270

merged 6 commits into from
Jul 21, 2016

Conversation

alexcjohnson
Copy link
Contributor

Fixes part of #259 - that array-valued parameters not only won't live plot from a Loop, they won't even sync correctly.

Changes proposed in this pull request:

  • better test of what rows of data are complete for saving and syncing. @AdriaanRol this probably won't even be an issue for HDF5 where each array is stored independently but in the meantime we need this fix.
  • snuck in a fix to MockInstrument complaining during snapshots

@nataliejpg can you verify that this fixes live plotting with the VNA?

removes the log message:
WARNING:root:Error getting or interpreting *IDN?: ''
We could also remove the IDN parameter from mocks, but maybe this will
be useful?
use modified_range and last_saved_index exclusively, rather than
looking for NaN in the data, which allows for array-valued parameters
where there may be lots of data in one column beyond the others
@nataliejpg
Copy link
Contributor

Yep, it works now 👍 Thanks Alex.

@giulioungaretti
Copy link
Contributor

I'd split the pr in two parts, the IDN on mock is totally unrelated and could just be pushed straight to master :D

@alexcjohnson
Copy link
Contributor Author

I'd split the pr in two parts, the IDN on mock is totally unrelated and could just be pushed straight to master :D

Yes, that would have been cleaner. But I don't like pushing straight to master, even for uncontroversial things like this, because then nobody sees the change.


If it's not complete, back up one point so that we don't need
to rewrite this point later on when it *is* complete
def _match_save_range_whole_file(self, arrays, only_complete):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@giulioungaretti is this a warning you consider worthwhile to include? I suppose we could "fix" it by pulling this function out of the class (bad! makes it harder to read) or probably by turning this into a @staticmethod but to me that just seems distracting for not much benefit, so I'd vote to turn this warning off unless you have a strong opinion otherwise.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's super super important actually :D And to suppress the warning with @staticmethod is the way I would go ! It's very pythonic 🐍

For me it's a clear message that the method/function is pure, and has no relation to the state of the object but just a logical relation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I can see that argument... for my own part I would have preferred not to introduce a whole new concept (@staticmethod) just to signal this, but OK.

@giulioungaretti
Copy link
Contributor

Ok, then let's maybe discuss this in another place, but then for which kind of branching model are we aming for ? Idk, a well written commit, it's always there in the logs, and the non git-fu people get a notification on slack :D

@alexcjohnson
Copy link
Contributor Author

@giulioungaretti OK to 💃 now?

@giulioungaretti
Copy link
Contributor

@alexcjohnson could be the perfect time to fix docstrings ? :P
depending on how pressing merging is.

@giulioungaretti
Copy link
Contributor

💃 X 🍦

@alexcjohnson alexcjohnson merged commit 010c681 into master Jul 21, 2016
@alexcjohnson alexcjohnson deleted the hotfix/live-plot branch July 21, 2016 20:27
@qcodes-bot
Copy link

Codacy Here is an overview of what got changed by this pull request:

Issues
======
+ Solved 1


Complexity increasing per file
==============================
- qcodes/tests/test_format.py  3

See the complete overview on Codacy

giulioungaretti pushed a commit that referenced this pull request Jul 21, 2016
Merge: 89d7a2c 249f7a4
Author: alexcjohnson <[email protected]>

    Merge pull request #270 from qdev-dk/hotfix/live-plot
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.

4 participants