-
Notifications
You must be signed in to change notification settings - Fork 20
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
Support for discrete geometries in netCDF output #459
Conversation
…t for nc just as for the other converters.
…. added option to docs.
Thank you much @huard. I'll dive into this over the next few days. |
@huard, could you enable push access to this PR at your leisure? It would be nice to get minor edits as part of the merge... Thanks! |
src/ocgis/ops/parms/definition.py
Outdated
@@ -72,6 +72,7 @@ class Aggregate(base.BooleanParameter): | |||
meta_false = 'Selected geometries are not aggregated (unioned).' | |||
|
|||
|
|||
# This is obsolete following new netCDF driver with support for discrete geoms. |
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.
We're going to want to leave this in there in case of future logic associated with this argument. It's also used for operations metadata.
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.
The allow edit from maintainers was checked. You should have access but I've added you to the collaborators for my fork.
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.
Comment removed.
@@ -775,6 +776,153 @@ def test_nc_conversion_multivariate_calculation(self): | |||
finally: | |||
ds.close() | |||
|
|||
|
|||
def test_nc_discrete_geometry_simple(self): |
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.
Nice testing! 👍
src/ocgis/conv/base.py
Outdated
def _get_or_create_shp_folder_(self): | ||
path = os.path.join(self.outdir, 'shp') | ||
if not os.path.exists(path): | ||
os.mkdir(path) | ||
return path | ||
|
||
def _preformatting(self, i, coll): |
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.
Should name this _preformatting_
since it is a private method.
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.
Fixed.
src/ocgis/conv/nc.py
Outdated
#TODO: UGID and GID show up in the output file, but they are equal. Remove one. | ||
|
||
# Size of spatial dimension | ||
if self.ops.aggregate is 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.
This is where it bails for the standard netCDF-CF-grid output, correct?
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.
Correct. The idea was to have preformatting perform whatever operation is needed on the collection before sending it to the driver. For the CF-grid, there is nothing to do, so it just returns coll as is.
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.
Makes sense. Thanks.
@@ -29,6 +37,9 @@ class NcConverter(AbstractCollectionConverter): | |||
+------------------------+----------------------------------------------------------------------------------------------------------------------------------------+ | |||
| unlimited_to_fixedsize | If ``True``, convert the unlimited dimension to fixed size. Only applies to time and level dimensions. | | |||
+------------------------+----------------------------------------------------------------------------------------------------------------------------------------+ | |||
| geom_dim | The name of the dimension storing aggregated (unioned) outputs. Only applies when ``aggregate is True``. | | |||
+------------------------+----------------------------------------------------------------------------------------------------------------------------------------+ |
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 do you think about adding an optional keyword argument defining an attribute on the geometry unique identifier that provides a name, path, etc. to the source geometry data? Maybe it makes more sense to put this info on the history attribute.
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 think that would be smart. The history attribute often ends up very crowded, I suggest saving that information into a global attribute. Could that be done automatically, that is without entering the information in the options but just by parsing the geom information ?
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.
It could be done with a little work to the geometry input (maintaining the path for example). However, there are many cases where the geometry will be a bare shapely geometry or bounding box. Also, one might not want to append a full path to the geometry file, etc. I think it should be something provided by the user or, at the very least, turned off. Can do some thinking on what would be best and can definitely add it later.
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.
Right. Note that I'm copying over the geom attributes into netCDF variables, so that's another place the info could appear.
@huard, there are some Python 3 related failures: https://travis-ci.org/NCPP/ocgis/jobs/269585613#L2065. I think you should be able to just do a |
Looking good! I'll make a push to expand on the output format documentation you added in the next couple days. |
- Moved additional output format descriptions to the appendix - Minor formatting
@huard This is ready to merge. I thought that the additional output format descriptions fit better in the appendix so moved them there and deleted the |
All good. I may have put description before calculate, but that's a minor point. |
Ha, yeah, that makes way more sense. |
Thanks! |
Supports netCDF output for multiple geometries, provided that aggregate is True. Instead of saving the results on an x/y grid, the results are stored along one geometric dimension. This would allow users to, for example, save the monthly mean temperature per country in a single file.