docstring & typehints in simplify.network_simplify(), etc.
#107
Conversation
sgeop/simplify.py
Outdated
| The factor by which singles, pairs, and clusters are simplified. | ||
| This value is multiplied by ``max_segment_length``. |
There was a problem hiding this comment.
This can take a bit to understand. In my head it is the vice versa, the max_segment_length is multiplied by this factor to get the simplification epsilon.
sgeop/simplify.py
Outdated
| First option threshold used to determine face artifacts. | ||
| Passed into ``artifacts.get_artifacts()``. |
There was a problem hiding this comment.
This is not clear.
When artifact_threshold is passed, we don't compute it from artifacts and just use the given value. This is useful for small networks where artifact detection may fail or become unreliable.
There was a problem hiding this comment.
Also, given artifacts.get_artifacts() and everything else mentioned here is essentially private, I am not sure public docstring should point to that.
sgeop/simplify.py
Outdated
| First option threshold used to determine face artifacts. | ||
| Passed into ``artifacts.get_artifacts()``. | ||
| artifact_threshold_fallback : None | float | int = None | ||
| Second option threshold used to determine face artifacts. |
There was a problem hiding this comment.
And this is used as a fallback. So we try to detect artifact threshold from the data and use that one but if that fails, we fall back to the value given here.
sgeop/simplify.py
Outdated
| Areal theshold for block detection. | ||
| Passed into ``artifacts.get_artifacts()``. | ||
| isoareal_threshold_blocks : float | int = 0.5 | ||
| Isoareal theshold for block detection. | ||
| See ``esda.shape.isoareal_quotient``. | ||
| Passed into ``artifacts.get_artifacts()``. | ||
| area_threshold_circles : float | int = 5e4 | ||
| Areal theshold for circle detection. | ||
| Passed into ``artifacts.get_artifacts()``. | ||
| isoareal_threshold_circles_enclosed : float | int = 0.75 | ||
| Isoareal theshold for enclosed circle detection. | ||
| See ``esda.shape.isoareal_quotient``. | ||
| Passed into ``artifacts.get_artifacts()``. | ||
| isoperimetric_threshold_circles_touching : float | int = 0.9 | ||
| Isoperimetric theshold for enclosed circle touching. | ||
| See ``esda.shape.isoperimetric_quotient``. | ||
| Passed into ``artifacts.get_artifacts()``. |
There was a problem hiding this comment.
All these will eventually, on the public API need more explanation on what they do and why they exist.
There was a problem hiding this comment.
I'll create a ticket dedicated to that.
| """Top-level workflow for simplifying networks. The input raw road network data is | ||
| first preprocessed (topological corrections & node consolidation) before two | ||
| iterations of artifact detection and simplification. For further information on | ||
| face artifact detection and extraction see :cite:`fleischmann2023`. |
There was a problem hiding this comment.
This will probably need way more but I am happy to craft that documentation at some point.
There was a problem hiding this comment.
Ya, I intended this a first minimal pass to get the process started. With that in mind, is there any more of an explanation to be added in this iteration?
This PR:
simplify.network_simplify()Once we are satisfied with the wording, I will propagate docstrings & typehints in the internal functions in a follow up PR.