3dgeo-heidelberg / py4dgeo

py4dgeo - A Python library for change analysis in 4D point clouds
https://py4dgeo.readthedocs.io
MIT License
65 stars 8 forks source link

Setting force = True in RegionGrowingAlgorithmBase.run() deletes smoothed_distances #318

Open hdaan opened 4 months ago

hdaan commented 4 months ago

To delete previous seeds and 4D objects-by-change (4D-OBCs) in the analysis object when running the 4D-OBC algorithm, one can set force to True.

The function then calls the invalidate_results() function, without any parameters. The default then deletes the smoothed_distances: https://github.com/3dgeo-heidelberg/py4dgeo/blob/bd7039430bae76accc06c9a42380a721c596aefd/src/py4dgeo/segmentation.py#L697C1-L697C42

This seems to be unwanted behavior, as one probably wants to use the smoothed_distances when computing 4D-OBCs, even if previous seeds and 4D-OBCs are preferably deleted.

A solution would be to either specify which attributes of the analysis object to delete in the invalidate_results function call, or change the default behavior of the invalidate_results function, to not delete the smoothed_distances.

kathapand commented 4 months ago

I think the preferred way is to change the default parameters of invalidate_results() and would support this, as I agree that in most cases users will want to re-run solely the seed detection and segmentation: def invalidate_results(self, seeds=True, objects=True, smoothed_distances=False) here

If parametrizing the function within the run() method, there would be two different behaviors when the function is called automatically or standalone, which could lead to confusion in usage.

A solution at the moment, without changing the source code, is to call invalidate_results() before run() and setting the parameters as wanted:

analysis.invalidate_results(smoothed_distances=False)
analysis.run()
dokempf commented 4 months ago

I do not have strong feelings here. Also, as using precalculated smoothed distances is purely a performance optimization, I do not see too big issues with backwards compatibility. (Also we are in the wild west that is v0.*)

kathapand commented 4 months ago

@hdaan could you implement the suggested change and make a pull request?