-
Notifications
You must be signed in to change notification settings - Fork 16
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
Add load_geojson #427
Add load_geojson #427
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.
some more nitpicking notes (not necessarily to block merging current state already)
proposals/load_geojson.json
Outdated
}, | ||
{ | ||
"name": "properties", | ||
"description": "A list of properties from the GeoJSON file to construct an additional dimension from. A new dimension is created if at least one property is provided. Only applies for GeoJSON Features and FeatureCollections. Missing values are generally set to no-data (`null`).\n\nDepending on the number of properties provided, the process creates the dimension differently:\n\n- Single property with scalar values: A single dimension label with the name of the property and a single value per geometry.\n- Single property of type array: The dimension labels correspond to the array indices. There are as many values and labels per geometry as there are for the largest array.\n- Multiple properties with scalar values: The dimension labels correspond to the property names. There are as many values and labels per geometry as there are properties provided here.\n\nDepending on the number of properties the dimension name is:\n\n- Single property: Name of the property\n- Multiple properties: `properties`.", |
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.
Some notes about the "properties" parameter. A bit nitpicking, so not necessarily blockers to merge already (can be moved to follow up ticket).
Single property with scalar values: ...
Single property of type array: ...
This assumes the a feature collection is internally consistent: the property is available for each geometry and it has same type for each geometry. When this is not the case: should this raise an error? Or should we describe type coercion rules: e.g. missing/null -> empty array, scalar value -> array with one item, so that everything can be described in terms of arrays.
Depending on the number of properties the dimension name is:\n\n- Single property: Name of the property\n- Multiple properties:
properties
.
I find it a bit strange to change the dimension name based on number of labels. We don't do that with other dimensions I think
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 assumes the a feature collection is internally consistent: the property is available for each geometry and it has same type for each geometry. When this is not the case: should this raise an error? Or should we describe type coercion rules: e.g. missing/null -> empty array, scalar value -> array with one item, so that everything can be described in terms of arrays.
As we also discusssed in the last meeting, openEO allows mixed-type data cubes, so in theory it should not throw an error, but if a "back-end limitation" requires consistent types and they are not consistent, yes, the implementation should throw an error.
I find it a bit strange to change the dimension name based on number of labels. We don't do that with other dimensions I think
Changed to be always properties
(and type: 'other').
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.
If you think there's more room for improvements, please open an issue as I'll merge this now. Thanks :-)
Implements #346 and #415