- Notifications
You must be signed in to change notification settings - Fork15
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
Uh oh!
There was an error while loading.Please reload this page.
Conversation
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 |
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. |
Sure, I added a pair of tests to sql/poly.sql |
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 commentedAug 2, 2023 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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. |
Interesting. Thank you for testing that and reporting your findings,@ggnmstr. |
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 commentedAug 5, 2023 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
@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. |
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.
@ggnmstr Could you, please, start the commit description from the capital letter?
@vitcpp Done |
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.
Once the tests are passed, I approve it. One trivial request is to start the PR title with a capical letter.@ggnmstr Thank you!
Uh oh!
There was an error while loading.Please reload this page.
The problem fixed by this pull request is described in Issue#35