PDOK-14231 #20

DKKepinski · 2022-07-06T06:11:24Z

Omschrijving

Als OZON wil ik uitgefilterde geometrieën geaggregeerd kunnen weergeven (POC)

geometrieen worden omgezet in POINT geom ipv verwijderd

https://dev.kadaster.nl/jira/browse/PDOK-14231

Type verandering

Nieuwe feature
Verbetering oude feature
Refactor
Breaking change (opzet/werking veranderd)

Checklist:

Ik heb de code in deze PR zelf nogmaals nagekeken
Ik heb mijn code beter achtergelaten dan dat ik het aantrof
De code is leesbaar en de moeilijke onderdelen zijn voorzien van commentaar
Ik heb de tests toegevoegd/uitgebreid indien nodig
Ik heb de tests gedraaid die de werking van mijn wijziging bewijst
De PDOK documentatie is bijgewerkt indien nodig.
Er zit geen gevoelig informatie in deze PR (wachtwoorden etc)

Als OZON wil ik uitgefilterde geometrieën geaggregeerd kunnen weergeven (POC)

roelarents · 2022-07-06T07:18:18Z

pkg/gpkg/gpkg.go

@@ -28,6 +28,10 @@ func (f *featureGPKG) UpdateGeometry(geometry geom.Geometry) {
 	f.geometry = geometry
 }

+func (f *featureGPKG) IsReduced(isReduced bool) {


misschien SetIsReduced of AddIsReduced

goeie, gefixed

WouterVisscher · 2022-07-06T18:45:48Z

om maar te beginnen... volgende keer iets vriendelijker branch name....
dit wil ik nooit meer tegenkomen "PDOK-14231_Als_OZON_wil_ik_uitgefilterde_geometrieën_geaggregeerd_kunnen_weergeven_(POC)" 😄

WouterVisscher · 2022-07-06T19:03:45Z

Als ik er nu zo over nadenk hebben we nu een sieve functie die "meer" doet dan alleen zeven.
Ik zou het zeven en vervangen graag terug zien in 2 aparte functies en die dan mogelijk te chainen met een channel
m.a.w. dat de solution van read->sieve->write dan iets kan worden als read->replace->sieve->write. Ook om op die manier te kijken/proberen of we de isReduced 'state' niet meer nodig hoeven te hebben.

m.b.t. de twpayne/go-geom package lijken er handige func() in te zitten, maar moeten we wel ff goed nadenken of we die moeten gebruiken of hoe we de bruikbare specifieke delen kunnen implementeren.

WouterVisscher · 2022-07-06T18:47:03Z

go.mod

@@ -9,6 +9,8 @@ require (
 	github.com/urfave/cli/v2 v2.8.1
 )

+require github.com/twpayne/go-geom v1.4.1


waarom een 2de spatial pkg?
is was go-spatial/geom niet voldoende?

ik heb de centroid berekening van de huidige als eerste geprobeerd, maar toen kreeg ik hele rare uitkomsten die ik niet kon verklaren. Infinity numbers, NaNs, hele hoge negatieve getallen (terwijl alle punten boven 0 waren) en als er een berekening uit kwam, dan klopte die niet. Na een dag proberen dacht ik even een ander pkg gebruiken, en die leverde wel output die ik verwacht (met als nadeel dat ik wat dingen moest doen om coordinaten om te zetten.

WouterVisscher · 2022-07-06T18:52:24Z

pkg/sieve.go

@@ -69,19 +74,54 @@ func writeFeaturesToTarget(postSieve chan Feature, kill chan bool, target Target
 	kill <- true
 }

+// getMultiPolygonCentroid returns Point with the centroid value of a multiPolygon


waar (behalve de test) wordt deze (getMultiPolygonCentroid) gebruikt?

ha, das inderdaad dubbel. die kan weg (opzet is paar keer gewijzigd)

WouterVisscher · 2022-07-06T18:58:13Z

pkg/sieve.go

+	return multiXyCoordinates
+}
+
+func makeCoord(point [2]float64) geometry.Coord {


hoe nodig is deze functie gezien die 1x maar wordt aangeroepen?

niet, maar ik dacht dat het leesbaarder zou zijn

WouterVisscher · 2022-07-06T19:07:25Z

pkg/sieve.go

 	for _, p := range mp {
-		if sievedPolygon := polygonSieve(p, resolution); sievedPolygon != nil {
+		if sievedPolygon, b := polygonSieve(p, resolution); b == false {


(b== false) -> !b

fair, gefixed

WouterVisscher · 2022-07-06T19:13:36Z

oja, voor z'n grote wijziging is het ook fijn als de README geupdate is (+voorbeeld)

DKKepinski · 2022-07-12T10:03:47Z

om maar te beginnen... volgende keer iets vriendelijker branch name.... dit wil ik nooit meer tegenkomen "PDOK-14231_Als_OZON_wil_ik_uitgefilterde_geometrieën_geaggregeerd_kunnen_weergeven_(POC)" smile

haha, nja, het is wel makkelijk te herleiden naar jira ticket zo ;) maar ik ga nadenken over hoe dat in de toekomst beter kan

Als ik er nu zo over nadenk hebben we nu een sieve functie die "meer" doet dan alleen zeven. Ik zou het zeven en vervangen graag terug zien in 2 aparte functies en die dan mogelijk te chainen met een channel m.a.w. dat de solution van read->sieve->write dan iets kan worden als read->replace->sieve->write. Ook om op die manier te kijken/proberen of we de isReduced 'state' niet meer nodig hoeven te hebben.

m.b.t. de twpayne/go-geom package lijken er handige func() in te zitten, maar moeten we wel ff goed nadenken of we die moeten gebruiken of hoe we de bruikbare specifieke delen kunnen implementeren.

dat is wat ik initieel wilde (sieve en replace functie los an elkaar), alleen dan zit ik met multipolygon die leunt op polygon sieve. omdat mp uit p bestaat, moet ik op dat punt al beslissen: weglaten of omzetten naar een punt.

het is mij niet helemaal duidelijk wat stap sieve in read-replace-sieve-write doet, we doen of weglaten of vervangen, maar dat is op basis van de berekeningen die in sieve functie gebeuren.

moeten we niet een situatie hebben zo van read->moet_ik_iets_mee?-(ja)->moet_ik_weglaten_of_vervangen->dat_gene_doen->write?

DKKepinski · 2022-07-12T10:12:47Z

Nog over de isReduced state: ik dacht dat het wenselijk was om zichtbaar te maken dat iets aangepast is. Het was iig handig om te hebben voor het testen ;)

Maar het leek mij ook netjes om te vermelden van "hey, deze informatie is bewerkt en niet 1-op-1 uit de data gehaald. Soort disclaimer eigenlijk

roelarents · 2022-08-15T12:35:03Z

pkg/sieve.go

 // multiPolygonSieve will split it self into the separated polygons that will be sieved before building a new MULTIPOLYGON
-func multiPolygonSieve(mp geom.MultiPolygon, resolution float64) geom.MultiPolygon {
+func multiPolygonSieve(mp geom.MultiPolygon, resolution float64) (geom.MultiPolygon, bool) {


kun je de return values namen geven? voor leesbaarheid en dan worden ze ook meteen geinitialiseerd

roelarents · 2022-08-15T12:35:57Z

pkg/sieve.go

+				mp := feature.Geometry().(geom.MultiPolygon)
+				mp, b := multiPolygonSieve(mp, resolution)
+
+				if b {


een beetje verwarrend (voor mij) dat de b hier net omgedraaid is t.o.v. die bij polygon. ook omdat hij in multiPolygonSieve needsProcessing genoemd is, maar de geometry hier dan juist niet in de needsProcessing channel gaat.

roelarents · 2022-08-15T12:40:29Z

pkg/sieve.go

+				mp := feature.Geometry().(geom.MultiPolygon)
+				var processedMultiPolygon geom.MultiPolygon
+				for _, p := range mp {
+					minArea := resolution * resolution


die kan minstens één regel omhoog

RoelvandenBerg · 2022-09-16T15:29:27Z

pkg/sieve.go

+	polygonCoords := getPolygonCoords(p)
+	if polygonCoords != nil {
+		polygon := geometry.NewPolygon(geometry.XY).MustSetCoords(polygonCoords)
+		centroidCoord, err := xy.Centroid(polygon)


waarom de centroide van de polygoon? en niet van de bbox?

PDOK-14231

ab40288

Als OZON wil ik uitgefilterde geometrieën geaggregeerd kunnen weergeven (POC)

DKKepinski requested review from WouterVisscher and roelarents and removed request for WouterVisscher July 6, 2022 06:11

roelarents reviewed Jul 6, 2022

View reviewed changes

WouterVisscher suggested changes Jul 6, 2022

View reviewed changes

Dominik Kepinski added 3 commits July 7, 2022 08:18

fix: close the geometry

d0ebe12

fix: close the geometry

ca8a9f9

fix: return point instead of multipoint

c9def7f

Dominik Kepinski added 5 commits July 18, 2022 12:29

split in sieve and processing

1c1e710

close readyToWrite after processing is done

1208d35

logic fault, reversed logical operator on multipolygon processing

6e39742

logic fault, reversed logical operator on sieve

779bc61

tests + readme

c4f9767

roelarents reviewed Aug 15, 2022

View reviewed changes

RoelvandenBerg reviewed Sep 16, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PDOK-14231 #20

PDOK-14231 #20

DKKepinski commented Jul 6, 2022 •

edited

Loading

roelarents Jul 6, 2022

DKKepinski Jul 12, 2022

WouterVisscher commented Jul 6, 2022 •

edited

Loading

WouterVisscher commented Jul 6, 2022 •

edited

Loading

WouterVisscher Jul 6, 2022

DKKepinski Jul 12, 2022

WouterVisscher Jul 6, 2022

DKKepinski Jul 12, 2022

WouterVisscher Jul 6, 2022

DKKepinski Jul 12, 2022

WouterVisscher Jul 6, 2022

DKKepinski Jul 12, 2022

WouterVisscher commented Jul 6, 2022

DKKepinski commented Jul 12, 2022

DKKepinski commented Jul 12, 2022

roelarents Aug 15, 2022

roelarents Aug 15, 2022 •

edited

Loading

roelarents Aug 15, 2022

RoelvandenBerg Sep 16, 2022

PDOK-14231 #20

Are you sure you want to change the base?

PDOK-14231 #20

Conversation

DKKepinski commented Jul 6, 2022 • edited Loading

Omschrijving

Type verandering

Checklist:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WouterVisscher commented Jul 6, 2022 • edited Loading

WouterVisscher commented Jul 6, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WouterVisscher commented Jul 6, 2022

DKKepinski commented Jul 12, 2022

DKKepinski commented Jul 12, 2022

Choose a reason for hiding this comment

roelarents Aug 15, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DKKepinski commented Jul 6, 2022 •

edited

Loading

WouterVisscher commented Jul 6, 2022 •

edited

Loading

WouterVisscher commented Jul 6, 2022 •

edited

Loading

roelarents Aug 15, 2022 •

edited

Loading