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

Add get_parameter_data aka get_columns #1400

Merged
merged 74 commits into from
Dec 20, 2018

Conversation

Dominik-Vogel
Copy link
Contributor

This pr add a function to the sqlite_base module and also a method to the DataSet, that allows to retrieve data in a similar way as get_data does, but instead of returning a list of rows it provides a much more useful dictionary whose keys are the parameter names and whose entries are the columns of the respective parameter as numpy arrays.

I changed the name to get_parameter_data in order to not use data storage representation specific terms such as 'column'.

This PR allows for an easy implementation of other export protocols.

It furthermore establishes the basis for a fast compiled implementation of the same.

@Dominik-Vogel Dominik-Vogel requested review from jenshnielsen and astafan8 and removed request for jenshnielsen November 28, 2018 15:47
@codecov
Copy link

codecov bot commented Nov 28, 2018

Codecov Report

Merging #1400 into master will increase coverage by 0.16%.
The diff coverage is 97.7%.

@@            Coverage Diff             @@
##           master    #1400      +/-   ##
==========================================
+ Coverage   74.09%   74.26%   +0.16%     
==========================================
  Files          85       85              
  Lines        9883     9946      +63     
==========================================
+ Hits         7323     7386      +63     
  Misses       2560     2560

Copy link
Collaborator

@jenshnielsen jenshnielsen left a comment

Choose a reason for hiding this comment

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

I will test this out further on some examples but I think this looks good

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

It looks good! Just needs to be tested for all the paramtypes, I guess.

Question: do I understand correctly that it is the sqlite_base.get_parameter_data function that is planned to be completely substituted with the compiled version? Not the sqlite_base.get_data?

Question: do I understand correctly that this PR intentionally ignores the fact that get_parameter_data is NOT faster than get_data?

@Dominik-Vogel
Copy link
Contributor Author

@astafan8 thank you!

  1. It will not be completely substituted, but it will rather form the entry point. Calling it results in looking up the presence of the compiled code. The query will still be build as is and then instead of executing it on python it will be executed from the compiled library. No,get_data will not be substituted. This is because getting the data in form of a dictionary of columns seems a lot more natural.
  2. Yes, it does ;-) As far as I can see there is no way in python to implement such a query faster. I have benchmarked some ideas but could not get any improvement.

@jenshnielsen
Copy link
Collaborator

I did a bunch of manual tests with combinations of array type and regular parameters. They seem to work great. I will transform them into regular tests. @Dominik-Vogel is it ok to push here?

@Dominik-Vogel
Copy link
Contributor Author

yes feel free @jenshnielsen !

@jenshnielsen
Copy link
Collaborator

@astafan8 I think this addresses all the issues you found

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 taking the effort of enhancing the tests and docstrings! It's also good to see that adding more tests didn't require significant changes in the implementation.

@jenshnielsen jenshnielsen merged commit 005d58e into microsoft:master Dec 20, 2018
giulioungaretti pushed a commit that referenced this pull request Dec 20, 2018
Merge: fb75a15 ef858af
Author: Jens Hedegaard Nielsen <jenshnielsen@gmail.com>

    Merge pull request #1400 from Dominik-Vogel/get_columns_pr
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

3 participants