Skip to content
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

Merged
merged 7 commits into from
Apr 3, 2023
Merged

Add load_geojson #427

merged 7 commits into from
Apr 3, 2023

Conversation

m-mohr
Copy link
Member

@m-mohr m-mohr commented Mar 31, 2023

Implements #346 and #415

proposals/load_geojson.json Outdated Show resolved Hide resolved
proposals/load_geojson.json Outdated Show resolved Hide resolved
@m-mohr m-mohr requested a review from soxofaan March 31, 2023 16:14
Copy link
Member

@soxofaan soxofaan left a 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 Show resolved Hide resolved
},
{
"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`.",
Copy link
Member

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

Copy link
Member Author

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').

Copy link
Member Author

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 :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Generalize load_geojson process to convert inline GeoJSON to a vector cube
4 participants