Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

Fix sline_sline_pos#36

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
vitcpp merged 1 commit intopostgrespro:masterfromggnmstr:spherepoly_check_fix
Aug 8, 2023

Conversation

ggnmstr
Copy link
Contributor

@ggnmstrggnmstr commentedJul 29, 2023
edited
Loading

The problem fixed by this pull request is described in Issue#35

@esabol
Copy link
Contributor

Always connect your pull requests to their relevant issue numbers, please. You can do this by mentioning the issue number in the title or description of the PR.

Issue#35

@ggnmstrggnmstr changed the titlefix sline_sline_posfix sline_sline_pos #35Jul 29, 2023
@ggnmstrggnmstr changed the titlefix sline_sline_pos #35fix sline_sline_posJul 29, 2023
@esabol
Copy link
Contributor

Seems like a simple enough change, but I'd like to see a test case added which fails without the PR and passes with the PR.

@ggnmstr
Copy link
ContributorAuthor

Sure, I added a pair of tests to sql/poly.sql
The first one definitely passes with the PR and fails without it (both polygons shouldn't be created).

dura0ok reacted with thumbs up emoji

@esabol
Copy link
Contributor

Looks good to me! Thanks,@ggnmstr!

What do you think,@vitcpp?

dura0ok, ggnmstr, and esabol reacted with thumbs up emoji

@vitcpp
Copy link
Contributor

Colleagues, I have some doubts about this change. I think, degenerate polygons can be accepted by most of the algorithms. This PR disables some types of polygons that may affect on user experience - if the old version constructed such degenerate polygons, the new version reports an error. Some user datasets may contain degenerate polygons. Explicit error reporting may stop data pipelines to process some datasets. Give me please some more time to think about it.

@esabol
Copy link
Contributor

esabol commentedAug 2, 2023
edited
Loading

This PR disables some types of polygons that may affect on user experience - if the old version constructed such degenerate polygons, the new version reports an error. Some user datasets may contain degenerate polygons. Explicit error reporting may stop data pipelines to process some datasets. Give me please some more time to think about it.

If there's a use-case for supporting degenerate polygons, I'd be open to it, but it seems to me that giving correct results is what should matter. Degenerate polygons are not valid polygons, according to the definition given by theOGC Simple Features Specification. Seehttps://cs.stackexchange.com/a/112703.

@ggnmstr
Copy link
ContributorAuthor

I found out that PGSphere is kind of inconsistent in terms of degenerate polygons. It doesn't allow to create all types of degenerate polygons:
before-patch
Meanwhile PR I suggest removes some possibilities to create degenerate polygons (connected by 0d coordinate), but not all:
after-patch
So it may be some kind of problem with these 0d coordinates, maybe math, not sure now.
But first, of course, we need to decide whether degenerate polygons should be allowed or not.

esabol reacted with thumbs up emoji

@esabol
Copy link
Contributor

Interesting. Thank you for testing that and reporting your findings,@ggnmstr.

ggnmstr reacted with thumbs up emoji

@vitcpp
Copy link
Contributor

It seems there is a more complex issue with degenerate polygons. Thank you@ggnmstr for your findings. There is the working idea to separate two concepts - storage and algorithms. spoly is a storage for polygon data. The storage may or may not validate the input data because in most cases the storage just used to manipulate data but not to validate it. My position is to allow to keep both valid and invalid data in storage. Data import algorithms just load data into storage objects as is. Some functions to validate and normalize data should be implemented as well. The algorithms assume that the input storage objects contain valid data. Such check is not trivial for polygons. It is the developer responsibility to pass valid data to algorithms. Otherwise, the behaviour is undefined. For most of the algorithms degenerate polygons are ok but it is important to avoid edge crosses that may lead to the wrong results. Overlapping edges may be used to construct polygons with holes (if needed).

Now we have some inconsistent behaviour with spoly. Once we can't create degenerate spoly with 4 points and it is the adopted behaviour I think I'm ok to disable creation of all types of degenerate polygons. Later, if we decide to allow degenerate polygons we can change this behaviour.

Lets wait for PR#22 and then merge it.

@vitcpp
Copy link
Contributor

vitcpp commentedAug 5, 2023
edited
Loading

@ggnmstr Could you please rebase the branch to the newer version and start the commit description from the capital letter? We will merge it if tests are passed.

Copy link
Contributor

@vitcppvitcpp left a comment

Choose a reason for hiding this comment

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

@ggnmstr Could you, please, start the commit description from the capital letter?

ggnmstr reacted with thumbs up emoji
@ggnmstr
Copy link
ContributorAuthor

@vitcpp Done

Copy link
Contributor

@vitcppvitcpp left a comment

Choose a reason for hiding this comment

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

Once the tests are passed, I approve it. One trivial request is to start the PR title with a capical letter.@ggnmstr Thank you!

@ggnmstrggnmstr changed the titlefix sline_sline_posFix sline_sline_posAug 7, 2023
@ggnmstr
Copy link
ContributorAuthor

Done.
@esabol,@vitcpp, thank you for review.

@vitcppvitcpp merged commit7e8a47d intopostgrespro:masterAug 8, 2023
@ggnmstrggnmstr deleted the spherepoly_check_fix branchAugust 9, 2023 03:19
@ggnmstrggnmstr restored the spherepoly_check_fix branchAugust 9, 2023 03:19
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@vitcppvitcppvitcpp approved these changes

@esabolesabolesabol approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

3 participants
@ggnmstr@esabol@vitcpp

[8]ページ先頭

©2009-2025 Movatter.jp