search

josephhardinee / PyDSD

Python Library for working with disdrometer data.
GNU Lesser General Public License v2.1
45 stars 33 forks source link

ENH: VDIS reader works with ARM TWP vdis files #69

Closed rcjackson closed 6 years ago

rcjackson commented 6 years ago

I noticed that when I tried to open up VDIS files from the ARM TWP b1 product using PyDSD's aux_reader module that the code to open up the files contained references to fields that did not exist in the file, most importantly delta_diam (which, in the TWP b1 files, is an attribute, not a variable). I also noticed that it made a reference to a variable rain_rate which was never created to begin with (and should have been).

Was this particular subroutine taylored towards a version of the data from another site? I would like to know before making this suggested modification to the main branch which makes it to where PyDSD can at least read in the vdis b1 files from TWP.

pep8speaks commented 6 years ago

Hello @rcjackson, Thank you for updating !

Cheers ! There are no PEP8 issues in this Pull Request. :beers:

Comment last updated on May 17, 2018 at 18:43 Hours UTC
coveralls commented 6 years ago

Coverage Status

Coverage decreased (-0.4%) to 60.64% when pulling ae1ffafed8a8d46e4cdd7a0154a07e80e46039eb on rcjackson:vdis into 9209b0fd6de295442ef11db4a876e8d09ae9331b on josephhardinee:master.

nguy commented 6 years ago

I'll let @josephhardinee chime in as well, but I think these changes stick to the common naming convention adopted. One things I would ask is to throw a message that would allow the user to know what variables were not found for troubleshooting purposes.

josephhardinee commented 6 years ago

It was tailored towards the datastreams back when I assumed they would be the same. I did a little hunting and chasing to try and make them all match up, but must have missed one. Can you stage a file somewhere (on stratus or DMF is fine) just so I can add some tests for this? I'll probably add a site argument to the reader to sort out some of these differences.

Overall the PR looks pretty good but I'll double check it tonight after work and get it merged in (Or possibly on the plane ride tomorrow).

rcjackson commented 6 years ago

I have left an example TWP VDIS file on stratus for testing: /lustre/or-hydra/cades-arm/proj-shared/twpdisdrometer/twpvdisC3.b1.20141116.000000.cdf

josephhardinee commented 6 years ago

Merging. I'll make some unit tests for this on a separate branch.