182 read logger files from solinst levelogger as obs #184
Conversation
New minor release v0.9.3
First phase of programming. Function can import an existing file now. Next step to test also other files (with other frequency, without x-y-location); and tests.
Update typo in brandname, should be Solinst (thanks @MattBrst)
|
|
||
| return cls( | ||
| df, | ||
| name=meta.pop("name"), |
There was a problem hiding this comment.
add full metadata dict first, then pop the items?
There was a problem hiding this comment.
I copied this working order from an other read package.
There was a problem hiding this comment.
Should probably also be changed there then, I think :). But i modified the order, so it should be fine now.
There was a problem hiding this comment.
Why do you want to change the order Davíd? I used this order because I don't want the metadata that is added as attributes in the 'meta' dictionary. But now that I think about it I think the order does not even matter.
There was a problem hiding this comment.
I thought you would want the metadata to be available both in the Obs object and as columns in the ObsCollection?
There was a problem hiding this comment.
But you say it doesn't matter? Does the dictionary get emptied regardless of the order you provide the keyword arguments?
There was a problem hiding this comment.
Just to be sure we have the same idea of what is happening: When we use x = meta.pop('x') we remove x from the meta dict and add x as an attribute to the obs object. Then we use meta=meta at the end to add the meta dict as the attribute meta to the Obs object. When we do it in this order the meta attribute of an Obs object does not have any keys that are also attributes of that Obs object.
I like this approach because I don't like to store the same information in multiple places. If we would change the order we might end up with an Obs object that has a meta attribute which is a dictionary with a key x and an attribute x which have the same value.
But then I am not sure what happens if you use a dictionary as the first function argument. I think a reference to that dictionary is passed to the function and not a copy so if you modify the dictionary in other function arguments (by popping items) I think it will have an influence the first argument. This should be easy to test.
I hope this makes it more clear. For now I think we can leave it as is and I can pick this up later
There was a problem hiding this comment.
Fair enough, I somehow thought it would be nice to have both metadata available as a single dictionary containing everything, and certain attributes for easy quick access. But I get that storing the same data in multiple locations is a bit redundant and maybe more confusing. So all good to keep things as they are, and maybe add a comment why the data is being popped :). For future me, or future someone else.
- black formatting - move import to top - zip one test file to test zip file read
|
test fails unrelated to this PR |
dbrakenhoff
deleted the
182-read-logger-files-from-soilinst-levelogger-as-obs
branch
Code and tests included.
Github suggested @dbrakenhoff as reviewer. Is that okay?