Repository navigation
fix(logbook_position_control): simplifier la géométrie avant terra::buffer() (perf + mémoire) - #14
Closed
JulienLebranchu wants to merge 1 commit into
Closed
JulienLebranchu wants to merge 1 commit into
JulienLebranchu wants to merge 1 commit into
Conversation
…() to cut runtime/memory terra::buffer() on unsimplified sea/harbour polygons dominates both runtime and native (off-R-heap) memory: measured 107s and ~800 MiB of process RSS on real production data (1130 activities, 49 trips), for buffered sea polygons reaching 110k-266k vertices after st_union(). That memory is invisible to gc() since it lives inside terra's GDAL/GEOS structures, and it is the direct cause of OOM kills in AkadoR/ShinyProxy sessions capped at 1 GiB (see #13 and akador-apps#49 on forge.ird.fr for the full investigation). Adds a `simplify_tolerance` parameter (default 50m, well below the existing buffer_sea=925/buffer_harbour=11100 defaults) applied via sf::st_simplify() before each terra::buffer() call. Set to 0 to disable. Verified on real data (1130 activities): ~4x faster (29.4s -> 7.1s), peak memory ~792 MiB -> ~499 MiB, and output is bit-for-bit identical (identical() == TRUE) to the unfixed version. Existing doctest unchanged and passing.
Collaborator
|
Bonjour Julien, |
Collaborator
|
Je ferme cette issue, le patch sf::st_simplify() est implémenté directement dans AkadoR. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #13.
Problème
logbook_position_control()appelleterra::buffer()sur les polygones de mer/ports à chaque exécution, sans simplification préalable. Sur des géométries fusionnées (st_union()de plusieurs sous-zones côté application appelante), ça produit des polygones à 100k+ sommets, et le buffering de ces géométries domine à la fois le temps d'exécution et la mémoire native (hors segment géré par R, donc invisible àgc()).Impact réel : ce coût est la cause directe des crashs OOM observés en production sur AkadoR (conteneurs ShinyProxy plafonnés à 1 Gio par session). Investigation complète : #13, et côté AkadoR akador-apps#49.
Correctif
Ajoute un paramètre
simplify_tolerance(défaut : 50 m, très inférieur auxbuffer_sea/buffer_harbourpar défaut de 925 m / 11100 m), appliqué viasf::st_simplify()juste avant chaque appelterra::buffer()(mer + ports départ/arrivée). Mettresimplify_tolerance = 0désactive la simplification (comportement historique).Vérification
Sur données réelles de production (1130 activités, 49 marées, 6 mois) :
report)identical()==TRUEavec la version non corrigéeLe doctest existant (
R/logbook_position_control.R/tests/testthat/test-doctest-logbook_position_control.R) passe sans modification — les géométries synthétiques du test sont trop petites pour que la simplification à 50 m ait le moindre effet.Non inclus dans cette PR
La deuxième piste identifiée dans #13 (mettre en cache le buffer calculé plutôt que de le recalculer à chaque appel) impliquerait un changement d'API plus large ; je la laisse à l'appréciation des mainteneurs comme évolution future, discutée dans #13.