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

Move write cache to from datasaver to dataset #2112

Merged
merged 5 commits into from Aug 14, 2020

Conversation

jenshnielsen
Copy link
Collaborator

First step in creating a more clean interface between the dataset and the datasaver

@codecov
Copy link

codecov bot commented Aug 13, 2020

Codecov Report

Merging #2112 into master will increase coverage by 0.03%.
The diff coverage is 88.07%.

@@            Coverage Diff             @@
##           master    #2112      +/-   ##
==========================================
+ Coverage   71.20%   71.23%   +0.03%     
==========================================
  Files         151      151              
  Lines       20148    20148              
==========================================
+ Hits        14346    14353       +7     
+ Misses       5802     5795       -7     

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.

Long time ago i made a mental model for myself for justifying the existence of DataSaver - it's that this object bridges DataSet that knows nothing about qcodes Parameters with the "measurement world" where poeple operate with qcodes parameters, including array parameters, multiparametersd, etc. So, with all that finalize_res_dict* stuff being moved here, which is done for the sake of DataSaver NOT having its "_results" cache, the point of DataSaver is largely gone.

So, my proposal would be to keep the finalize_res_dict* in DataSaver, and move only the "_results" part to dataset with the write_period and the flush_to_db method.

What do you think? Do you agree with the mental model?

as a side note - we can probably indeed move in the direction of getting rid of DataSaver but that's an API change, and i don't know if we'll do it anytime soon, so this is why i'm still folowing that mental model so that DataSaver has at least some justification for its existence :)

Pelase ignore this, i didn't read the PR well when i wrote it, sorry for that :)

qcodes/dataset/measurements.py Show resolved Hide resolved
qcodes/dataset/data_set.py Show resolved Hide resolved
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.

Approve! modulo guarding _enqueue_results with a check for DataSet state

qcodes/dataset/data_set.py Show resolved Hide resolved
qcodes/dataset/data_set.py Show resolved Hide resolved

return res_list

def flush_data_to_database(self, block: bool = False) -> None:
Copy link
Contributor

Choose a reason for hiding this comment

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

shall we keep this private? my thinking is if it's private now, then if we decide to change/remove/move it, we don't need to do deprecation cycles and stuff.

@jenshnielsen jenshnielsen merged commit 55b488b into microsoft:master Aug 14, 2020
@jenshnielsen jenshnielsen deleted the move_write_cache branch August 14, 2020 12:39
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

2 participants