-
Notifications
You must be signed in to change notification settings - Fork 322
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
Conversation
Codecov Report
@@ 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 |
There was a problem hiding this 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
There was a problem hiding this 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
?
@astafan8 thank you!
|
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? |
yes feel free @jenshnielsen ! |
That also works for strings.
start and end values
@astafan8 I think this addresses all the issues you found |
There was a problem hiding this 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.
Merge: fb75a15 ef858af Author: Jens Hedegaard Nielsen <[email protected]> Merge pull request #1400 from Dominik-Vogel/get_columns_pr
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.