netCDF Front End for Cloud Model Simulations #2176
Conversation
matthewturk
added
triage
|
@yt-fido test this please |
keltonhalbert
force-pushed
the
nc4-griddata
branch
from
8e363d7
to
0162faf
Compare
…to nc4-griddata
…cks all of the variables and their coordinates and compares them to the global dataset coordinates to make sure all coordinates are present and accounted for
…ordinates for each variable
|
@yt-fido test this please |
| self.domain_left_edge = np.array( | ||
| [xh.min(), yh.min(), zh.min()], dtype="float64" | ||
| ) | ||
| self.domain_right_edge = np.array( | ||
| [xh.max(), yh.max(), zh.max()], dtype="float64" | ||
| ) |
There was a problem hiding this comment.
Are xh, yh, zh the cell centers or the cell edges?
I would naively expect that if xh represents the cell edges, then the number of dimensions is ds.dimensions["xk"].size - 1, and if it represents the cell centers, then the left and right edge should be computed by adding -dx/2 and +dx/2 respectively.
I however see that you are using ds.dimensions and ds.variables, so there is definitely some subtlety that I am missing. In any case, it'd be worth commenting to say what the xh (as well as yh and zh represent).
There was a problem hiding this comment.
we'll need @keltonhalbert to weigh in on xh, yh and zh (I think they are actually cell centers but I'm not positive?). I think adding a comment to the code would be good here -- maybe including a link to the cm1_lofs docs website?
But I think some confusion is coming from the fact that ds here is our netcdf dataset file handle, not a yt dataset. So ds.variables contains the netcdf objects with the actual data that need to be parsed and read while ds.dimensions is a netcdf attribute with dimensionality metadata. I think this will be clearer by switching ds to a different name (_handle as Matt suggested).
There was a problem hiding this comment.
@chrishavlin @cphyc xh, yh, and zh are the cell center coordinates where scalars are defined.
There was a problem hiding this comment.
added to the comments around this section noting the xh, yh and zh coordinates.
|
|
||
| self.dimensionality = 3 | ||
| self.domain_dimensions = np.array(dims, dtype="int64") | ||
| self.periodicity = (False, False, False) |
There was a problem hiding this comment.
Is it always the case? Or could you imagine cloud models produced with your code with periodic boundary conditions? If this is likely, add a # TODO comment to say that periodicity should be supported, or even better (if possible) raise a NotImplementedError if periodicity is detected.
If no cloud model uses periodicity, then would you mind adding a comment to document it? Something like
| self.periodicity = (False, False, False) | |
| # Cloud Model Simulations do not support periodicity, so we hard-code it here. | |
| self.periodicity = (False, False, False) |
|
|
||
| warn_netcdf(args[0]) | ||
| try: | ||
| from netCDF4 import Dataset |
There was a problem hiding this comment.
Shouldn't it be our mock one from yt.utilities.on_demand_imports import _netCDF4 as netCDF4?
There was a problem hiding this comment.
I think it could be... this follows the exodus_ii is_valid() but I'm not sure if there was a reason for importing netcdf directly there either.
There was a problem hiding this comment.
oh, sorry, was conflating two issues. You just meant the import. Yes, should definitely be able to use the _netCDF4 import. But I've been confused about why the is_valid (here and in exodus_ii) did not use the NetCDF4FileHandler that is already imported. After more digging, the reason for that seems to be the need for the keepweakref argument, since NetCDF4FileHandler.open_ds uses hardcoded arguments for the call to the netcdf dataset. So I'm inclined to add **kwargs to NetCDF4FileHandler.open_ds so that we can simply pass along the keepweakref using the same NetCDF4FileHandler used in the rest of the front end.
There was a problem hiding this comment.
latest commit fixes this: uses the NetCDF4FileHandler and no longer imports from netCDF4 directly.
| if coordspassed != ncoords: | ||
| failed_vars.append(var) | ||
|
|
||
| if len(failed_vars) > 0: |
There was a problem hiding this comment.
It is a matter of taste, but I would rather show a warning than an error here. I'm fine if you think it should be an error.
| _fields = ("thrhopert", "zvort") | ||
| cm1sim = "cm1_tornado_lofs/nc4_cm1_lofs_tornado_test.nc" | ||
|
|
||
|
|
There was a problem hiding this comment.
Should an answer test be added here?
There was a problem hiding this comment.
test_cm1_mesh_fields is an answer test, ya?
There was a problem hiding this comment.
Overall this looks nice, well done !
I have a few minor questions and remarks
- this is committing an empty
misc.pyfile that should either be removed or populated - I'm confused that the
CM1Datasetclass has avariablesattribute that seems very close to whatparametersis supposed to be for. Is there a reason to have both ? - I'm not reviewing the tests for now. We would need to coordinate this with the testing framework change.
I'll happily approve this once my questions and suggestions are answered :)
|
Yes, it does.
…On Wed, Sep 30, 2020 at 9:51 AM chrishavlin ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In yt/frontends/nc4_cm1/data_structures.py
<#2176 (comment)>:
> + self.coordinates._x_pairs = (("x", "y"), ("y", "x"), ("z", "x"))
+ self.coordinates._y_pairs = (("x", "z"), ("y", "z"), ("z", "y"))
I believe this sets the axis ordering on slice plots so that when slicing
z is always on the vertical axis -- @matthewturk
<https://github.com/matthewturk> or @keltonhalbert
<https://github.com/keltonhalbert> should confirm...
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#2176 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAAVXO42LCRSLXUF3QPJXZLSINAYTANCNFSM4G35FIPA>
.
|
|
often we use "_handle"
…On Wed, Sep 30, 2020 at 9:53 AM chrishavlin ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In yt/frontends/nc4_cm1/data_structures.py
<#2176 (comment)>:
> + self.parameters["variable_names"] = varnames
+ self.parameters["lofs_version"] = ds.cm1_lofs_version
+ self.parameters["is_uniform"] = ds.uniform_mesh
ds here is the netcdf file handle, where ds.cm1_lofs_version is a global
attribute. Maybe we should use a different variable name from ds to avoid
confusion with a yt dataset?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#2176 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAAVXOZQKF5EYHK2CCACKZ3SINA7HANCNFSM4G35FIPA>
.
|
Co-authored-by: Clément Robert <cr52@protonmail.com> Co-authored-by: Corentin Cadiou <contact@cphyc.me>
|
just pushed some changes that incorporate most of the suggestions (but not all). will go through now and note the ones that still need addressing. |
There was a problem hiding this comment.
Great job @chrishavlin ! I like where this is going. Even though there are still some open questions that need addressing, I'll approve this to indicate the changes I requested were dealt with :-)
Co-authored-by: Clément Robert <cr52@protonmail.com>
|
added a note about the cut region bug in the existing issue #2720 |
|
Ok, awesome! I think this looks good to go, then, so I'll merge it and we can resolve the issue a bit down the road. 🙂 |
|
Hooray! Congratulations on your first merged pull request! We hope we keep seeing you around! 🎆 |
|
Congratulations on getting this frontend over the finish line @keltonhalbert and @chrishavlin !!! |
PR Summary
Added a front-end for reading CF compliant netCDF files from atmospheric cloud model simulations from the NCAR CM1 model. It uses xarray for disk io, but considering switching to the netCDF4 API due to extra/unnecessary dependencies required through xarray. This is in part to a dependency in order to import xarray within a try/except block.
Right now this will only work for netCDF files generated from CM1, as it depends on following the specific axis naming conventions. Ideally, this should be generalized such that any CF compliant netCDF can be read.
So far it seems like it handles the reading properly and plots - however, I am unsure of what tests should be conducted to make sure there aren't any issues hiding.