First ever pull request #9
Conversation
| try: | ||
| inv_df = feather.read_feather(inventory) | ||
| except: | ||
| feather.write_feather(inventory, 'temp.feather') |
There was a problem hiding this comment.
This line confuses me. I thought that inventory holds a file name and inv_df contains the (possibly empty) data structure. Here, you seem to be writing the file name to temp.feather
There was a problem hiding this comment.
When I first ran the download I tried to pass some non-"feathered" data, This was to check if it was feathered and if not feather it. This just made for a smoother transition so don't have to make sure the feathering is in your script, it will do it for you.
| """Given an inventory file (or dataframe as returned by inventory) | ||
| for a particular day, and a destination directory (default to current |
There was a problem hiding this comment.
Ah... so you are trying to make this function polymorphic.
That makes the type of inventory variable. The rationale for the old design was that we always wanted the inventory in a file because that allows one inventory fetch to cause many file downloads (for backfill).
Are you saying that you want to merge the inventory and download steps?
There was a problem hiding this comment.
I just wanted to have it be more flexible, I have been effectively merging the inventory and download steps while I was playing around with it. I didn't think what I added limited the ability for one inventory fetch to have many downloads to a file.
| def download(inventory, dest_dir, max_download=4): | ||
| """Given an inventory file for a particular day, and a destination | ||
| directory, download all files in the inventory that are not | ||
| def download(inventory, dest_dir = os.path.join(os.getcwd(),'Data'), max_download=4): |
There was a problem hiding this comment.
Our data is already in a lower-case data directory. Should we change that name (and all current workflows) or should we change this?
There was a problem hiding this comment.
Change this, The object of this was to just add the data directory as the default
|
I see.
How about a mezzanine function that prepares the ground for the download
function?
Or just a combo that calls both sides?
Also, there are command line versions that can be used for testing now.
…On Thu, Sep 23, 2021 at 12:01 PM gunnarnewell ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In pipeline/mrms_download.py
<#9 (comment)>:
> + """Given an inventory file (or dataframe as returned by inventory)
+ for a particular day, and a destination directory (default to current
I just wanted to have it be more flexible, I have been effectively merging
the inventory and download steps while I was playing around with it. I
didn't think what I added limited the ability for one inventory fetch to
have many downloads to a file.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#9 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAB5E6UBEQ3P4OVZKIXNPKDUDN2PLANCNFSM5DHVTJSA>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
|
Added default dest_dir and internalized the 'feathering' of the inventory file in mrms_download.py. This makes for a smoother from the inventory() functions to download() function