-
Notifications
You must be signed in to change notification settings - Fork 0
netCDFWriter #5
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
base: master
Are you sure you want to change the base?
netCDFWriter #5
Conversation
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.
Overall, looks great, thanks!
Only fix I ask is that the unprotected import throw a helpful pointer to pip'ing the requirements.txt, as in inline comments.
import sys | ||
|
||
# requires pip install netCDF4 | ||
from netCDF4 import Dataset |
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.
Can you wrap this in a "try" and tell the user what to do if they try to use this without the right packages? ("Please run pip install ...").
time_format=timestamp.TIME_FORMAT, | ||
date_format=timestamp.DATE_FORMAT, | ||
split_char=' ', suffix='', header=None, | ||
header_file=None, rollover_hourly=False, |
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.
Something I wish we'd done with LogfileWriter was to combine the date format with rollover_hourly. Basically, there would be a string that specified how the datetime was represented in the filename. As long as a new record's datetime matched the previous datetime, it would be put in the same file. So if the format were %Y-%M, the writing would roll over only when the year or month changed; if it were %Y-%M-%D-%H-%m, it would roll over every time the minute changed.
Using the current LogfileWriter calling convention is fine, but I wouldn't be disappointed if you had any extra cleverness left over to figure out how to sneak this functionality in...
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.
Good shout, I think i've managed to implement that (works in the tests on my main repo)
Adds a basic implementation of a netCDF file writer using the netCDF4 python library. This writer is heavily based on the existing logfile writer.
Netcdf supports more than this writer is doing, potential areas for further development are: