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

feat(custom-pu-grid): custom pu grid geometry #525

Merged
merged 6 commits into from
Sep 15, 2021

Conversation

kgajowy
Copy link
Contributor

@kgajowy kgajowy commented Sep 7, 2021

@vercel
Copy link

vercel bot commented Sep 7, 2021

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployments, click below or on the icon next to each commit.

marxan – ./app

🔍 Inspect: https://vercel.com/vizzuality1/marxan/DahoHLajqxJDvDcD5WsMz8csogW9
✅ Preview: https://marxan-git-feat-marxan-252-custom-pu-geometry-grid-vizzuality1.vercel.app

marxan-storybook – ./app

🔍 Inspect: https://vercel.com/vizzuality1/marxan-storybook/HW8MPLgX6VRKYVJDFPyG8ZDEhEsB
✅ Preview: https://marxan-storybook-git-feat-marxan-252-custom-945531-vizzuality1.vercel.app

@kgajowy kgajowy force-pushed the docs/custom-planning-unit-geometry-grid branch from cf7f3a4 to eeb1c5c Compare September 7, 2021 12:03
@kgajowy kgajowy force-pushed the feat/MARXAN-252-custom-pu-geometry-grid branch from 865bc7d to 960b192 Compare September 7, 2021 12:27
@kgajowy kgajowy changed the base branch from docs/custom-planning-unit-geometry-grid to develop September 7, 2021 12:28
@kgajowy kgajowy force-pushed the feat/MARXAN-252-custom-pu-geometry-grid branch from 960b192 to 159b8d3 Compare September 8, 2021 07:49
@kgajowy kgajowy force-pushed the feat/MARXAN-252-custom-pu-geometry-grid branch from 159b8d3 to 18bd3bd Compare September 8, 2021 07:52
@kgajowy kgajowy force-pushed the feat/MARXAN-252-custom-pu-geometry-grid branch from 18bd3bd to 7db80ea Compare September 8, 2021 09:59
@kgajowy kgajowy force-pushed the feat/MARXAN-252-custom-pu-geometry-grid branch from 7db80ea to d48fbed Compare September 8, 2021 10:15
Copy link
Member

@aagm aagm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In terms of validation, it is part of the characteristic of a grid that their members are adjacent geometries. in order to validate it, they should not intersect with each other, but every single geometry should be intouch (connected) with at least one. If we want to do that validation with javascript, i think turf will allow you to intersect and check if the result is a line in order to ensure that they only share borders.

FROM (
SELECT json_array_elements($1::json -> 'features') AS features
) AS f
ON CONFLICT ON CONSTRAINT planning_units_geom_the_geom_type_key DO UPDATE SET type = 'irregular'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

type should be inputted from the FE

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, it's a pity we haven't mentioned this in requirements.

Are we sure this makes sense? Allowing user to provide whatever shape they want and marking the as "hex" (which isn't correct, right?) may lead system to produce unexpected results.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The user will upload their custom planning grid, which could be a regular (square, hexagon, triangles...) or an irregular one( whatever continuos group of geometries that can represent basins, parcels...).
For those regulars, the main difference with the ones we create is related to the grid upper left corner origin. If we want to be agnostic to the type we can make use of the same categorization in the enum planning_unit_grid_shape and mark them as from_shapefile.
But marking it as irregular is just wrong. What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@aagm I think following #526 (comment) at this very moment would be fine. (in future we can make the validation/checks also instead asking for a type, we will need to verify it anyway so we can also deduce it, right?) Nevertheless, as long as it isn't that much concern for this moment, from_shapefile sounds fair enough for current state.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated in 0503960

@kgajowy
Copy link
Contributor Author

kgajowy commented Sep 9, 2021

In terms of validation, it is part of the characteristic of a grid that their members are adjacent geometries. in order to validate it, they should not intersect with each other, but every single geometry should be intouch (connected) with at least one. If we want to do that validation with javascript, i think turf will allow you to intersect and check if the result is a line in order to ensure that they only share borders.

Can we validate this assumption with postgis?

would https://postgis.net/docs/ST_IsSimple.html be fair enough (not sure about mentioned tangency) - checked, seems like not, as touching edges make it "invalid"

https://postgis.net/docs/postgis_usage.html#eval_spatial_rel

@hotzevzl
Copy link
Member

In terms of validation, it is part of the characteristic of a grid that their members are adjacent geometries. in order to validate it, they should not intersect with each other, but every single geometry should be intouch (connected) with at least one. If we want to do that validation with javascript, i think turf will allow you to intersect and check if the result is a line in order to ensure that they only share borders.

Can we validate this assumption with postgis?

would https://postgis.net/docs/ST_IsSimple.html be fair enough (not sure about mentioned tangency) - checked, seems like not, as touching edges make it "invalid"

https://postgis.net/docs/postgis_usage.html#eval_spatial_rel

not something I have much experience with, but I would be surprised if this wasn't the case, and it would be much more preferable than doing this at the API layer. We can deal with this in a separate validation story, if effectively needed.

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.

3 participants