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

Geo 5769 tolerance per geom type #1603

Merged
merged 5 commits into from
Sep 1, 2022
Merged

Conversation

mki-c2c
Copy link
Contributor

@mki-c2c mki-c2c commented Aug 29, 2022

New possibilities for configuration:
either leagsxy conf: tolerance + scalar
or new conf per geometry:
{"Point": 0.1, "Polygon": 0.05}
or dict for all:
{"ALL": 0.2}

a different value can be defined per geometry type
and add retro compatibility
either tolerance + scalar
or tolerances + dict may be used
@mki-c2c mki-c2c requested a review from jwkaltz August 29, 2022 07:41
@codecov
Copy link

codecov bot commented Aug 29, 2022

Codecov Report

Merging #1603 (39976cc) into master (99095f8) will decrease coverage by 0.05%.
The diff coverage is 77.41%.

@@            Coverage Diff             @@
##           master    #1603      +/-   ##
==========================================
- Coverage   76.52%   76.47%   -0.06%     
==========================================
  Files         127      127              
  Lines        5185     5202      +17     
==========================================
+ Hits         3968     3978      +10     
- Misses       1217     1224       +7     
Flag Coverage Δ
unittests 76.47% <77.41%> (-0.06%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
pyramid_oereb/core/records/geometry.py 90.76% <62.50%> (-4.15%) ⬇️
...b/contrib/data_sources/interlis_2_3/sources/plr.py 81.76% <75.00%> (-0.36%) ⬇️
...oereb/contrib/data_sources/standard/sources/plr.py 51.05% <100.00%> (+0.78%) ⬆️
pyramid_oereb/core/records/plr.py 100.00% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@jwkaltz jwkaltz requested a review from marionb August 29, 2022 08:36
Copy link
Member

@jwkaltz jwkaltz left a comment

Choose a reason for hiding this comment

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

Thanks @mki-c2c , the code looks good to me;
could we add an example configuration to https://github.com/openoereb/pyramid_oereb/blob/master/dev/config/pyramid_oereb.yml.mako ?

@mki-c2c
Copy link
Contributor Author

mki-c2c commented Aug 30, 2022

@jwkaltz : you mean a commented stub in the config ? Because I am a bit afraid to put a default tolerance which changes quite significantly the search behaviour. So we might put some comments with a default configuration which can be uncommented by someone who wants to use this config ?

@jwkaltz
Copy link
Member

jwkaltz commented Aug 30, 2022

@jwkaltz : you mean a commented stub in the config ? Because I am a bit afraid to put a default tolerance which changes quite significantly the search behaviour. So we might put some comments with a default configuration which can be uncommented by someone who wants to use this config ?

Yes you're right, the "standard configuration" shouldn't have that enabled.
So adding an example configuration for one of the themes (the first one which is of type geometrycollection is probably best), but commenting it out, seems to be the best approach.

Copy link
Member

@jwkaltz jwkaltz left a comment

Choose a reason for hiding this comment

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

Thanks this looks good, but please undo the change of print base_url (not relevant here)

@@ -48,7 +48,7 @@ pyramid_oereb:
# Define whether all geometry data must be included when sending the data to the print service
with_geometry: True
# Base URL with application of the print server
base_url: https://oereb-dev.gis-daten.ch/oereb/report/create
base_url: https://oereb-pdf.gis-daten.ch/oereb/report/create
Copy link
Member

Choose a reason for hiding this comment

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

This shouldn't be in this PR

@jwkaltz jwkaltz requested a review from svamaa August 30, 2022 13:38
@jwkaltz
Copy link
Member

jwkaltz commented Aug 30, 2022

@svamaa perhaps this new feature is relevant for Basel-Stadt as well? As I understand, it addresses the need that @svareg had a while ago.

@jwkaltz jwkaltz requested review from voisardf and vvmruder August 30, 2022 13:41
Copy link
Collaborator

@vvmruder vvmruder left a comment

Choose a reason for hiding this comment

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

Looks ok code wise. About the tolerance check itself I fell still not convinced. But you get an approve.

@jwkaltz jwkaltz merged commit 8238f6e into master Sep 1, 2022
@jwkaltz jwkaltz deleted the GEO-5769_tolerance_per_grom_type branch September 1, 2022 08:46
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