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

Fix bug in exclusion of small water bodies #601

Merged
merged 1 commit into from
Aug 30, 2023

Conversation

quaelnix
Copy link
Collaborator

This little pond was causing estimated_river_class=2 on the Hackländerstraße:

estimated_river_class


It would probably also be worth discussing whether it would not make sense to raise the threshold back to something near the original value of around 8000 square meter:

-- and st_area(q.way) > 90746 !!! filter on very small surfaces was set above !!!!!!!!!

@quaelnix quaelnix added the bug label Jul 27, 2023
@quaelnix quaelnix temporarily deployed to BRouter July 27, 2023 16:41 — with GitHub Actions Inactive
@afischerdev
Copy link
Collaborator

Thanks for your fix.
But please hang on a moment, @EssBee59 will not comment at the moment.
And I have the task of checking in the latest changes.

@quaelnix
Copy link
Collaborator Author

quaelnix commented Aug 27, 2023

@afischerdev, it's been a month now with zero input on this.

Why don't we fix this bug and what prevevents us from going back to a sane threshold of around 8000 square meters?

@afischerdev
Copy link
Collaborator

@quaelnix
I don't mind to do that. But I thought maybe @EssBee59 would like to add something else to that.

@EssBee59
Copy link
Collaborator

EssBee59 commented Aug 28, 2023

It's ok to have different opinions, but I don't understand to create an issue with the label "bug" directly out of it!

I prefer to keep 1000 qm, else a lot of nice water places would not be considered, and this would probably lead to the report of new bugs.
Examples:

case1
case2

@quaelnix
Copy link
Collaborator Author

I prefer to keep 1000 qm

This PR fixes the bug that the threshold is currently ~ 1000 square feet instead of 1000 square meters.

@quaelnix
Copy link
Collaborator Author

Another example. This little basin (less than 300 m² in size) is causing estimated_river_class=4 on Wiesenstraße:

@afischerdev
Copy link
Collaborator

This PR fixes the bug that the threshold is currently ~ 1000 square feet instead of 1000 square meters.

I'm not sure about this.
A sql like this
select p.osm_id, p.water, st_area(p.way), st_area(st_transform (p.way, 4326)::geography) area_4326, st_area(st_transform (p.way, 4326)::geography) / 0.3048 ^ 2 sqft_4326 from polygons p where p.water is not null limit 10;
brings out e.g this pont - controlled by this - switch to satellite view

osm_id water st_area area_4326 sqft_4326
833830724 pond 395.8 154.1 1659.6

Seems the area_4326 is the correct value for sqr meter.

@quaelnix
Copy link
Collaborator Author

quaelnix commented Aug 29, 2023

@afischerdev, yes, I got the square feet wrong, but that doesn't change the fact that the current code never gives a threshold of 1000 square meter unless you are at the equator.

Essbee59 wants a 1000 square meter threshold, I'm fine with a 1000 square meter threshold and this pull request does get us a 1000 square meter threshold, so please let us merge it.

@EssBee59
Copy link
Collaborator

Yes, 1000 is at the equator!
Historically the first calculation were done for Europe only...
Yes I can / will next change that in the same way as I did by nature_reserve (in Greenland, there was a real need for that)

It seems extremly important for you to be "exact" by a parameter that can just be set with feeling / testing...
I would prefer to concentrate my energy on really critical points (as #622)
Regards

@quaelnix
Copy link
Collaborator Author

that can just be set with feeling / testing...

Yes, but the intention was 1000 square meters and not < 300 square meters. That's the whole point of this pull request.

I would prefer to concentrate my energy on really critical points

Me too, see: #614

@afischerdev afischerdev merged commit 88ec15f into abrensch:master Aug 30, 2023
1 check passed
@quaelnix quaelnix deleted the fix-st-area branch August 30, 2023 16:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants