-
Notifications
You must be signed in to change notification settings - Fork 2
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
Complete dask demo #26
Conversation
boss level, @davidhassell 🍺 Did you want to add a test that runs the demo or some unit/integration tests? No probs if not, we can do that later 👍 |
Cheers, @valeriupredoi. Don't mind either way on the tests question. |
self.ncvar = ncvar | ||
|
||
nc = netCDF4.Dataset(self.filename, "r") | ||
v = nc.variables[self.ncvar] |
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.
What happens if the keyword ncvar is not present? Would it be better to pass a NetCDF dataset (already instantiated) in along with a variable name (both required)?
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.
(There would be some consequences. I would work through them, but you will have seen I have a git problem right now.)
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.
You'll get an exception with no ncvar
or no filename
. I don't think that it makes sense to pass in a netCDF.Dataset
instance, as on principle we don't want those hanging around as open file handles, but we could make the __init__
args positional only. I'll change that.
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'm happy with this. @valeriupredoi Can you please merge.
cheers, gents! 🍺 |
Hi, not much to say here, other than I have address the commenting points discussed on Thursday. Also
cfdm
is no longer a dependency, as I now instantiate things indemo.py
as