Dump optimized thresholds for buildings as a yaml file #109
Conversation
| TMP_DIR = Path("tmp/lidar_prod/tasks/building_validation_optimization") | ||
|
|
||
|
|
||
| def setup_module(module): |
There was a problem hiding this comment.
Could you consider using a temporary dir obtained using tempfile instead? Or is there a reason to hardcode this?
There was a problem hiding this comment.
the reason decided to hardcode this is to be able to have a look at the generated results (mainly in case the test fails)
| TMP_DIR.mkdir(parents=True, exist_ok=True) | ||
|
|
||
|
|
||
| def test_BuildingValidationOptimizer_run(hydra_cfg): |
There was a problem hiding this comment.
This test, while simpler, seems redundant with other tests for the BVO in https://github.com/IGNF/lidar-prod/blob/yaml-thresholds/tests/lidar_prod/test_optimization.py
It also seems that you added new optimization files to /tests/files/ instead of relying on the previous ones. Is there a reason to this?
It's probably a good reason to merge the two, and to assert the existing of output files like you do in this version.
There was a problem hiding this comment.
I don't understand the point of merging both tests: if that test fails we know it's the save/load function, if the other fails we know it's something else. What would be the point of merging?
There was a problem hiding this comment.
The point is just to avoid duplicate code by adding this new assertion on the threshold file to the already existing test
There was a problem hiding this comment.
However, I'd rather move the test on BuildingValidationOptimizer to test_building_validation_optimization.py to ensure consistency between the test files directory and the library files directory
There was a problem hiding this comment.
The change of file format itself seems ok. However, I think you may be duplicating tests and test data for the optimization of thresholds.
I suggest simplifying the test, perhaps by simply adding more test assertions to the existing ones in test_optimization.
Current structure separes end-to-end tests (test_application,tesrt_optimization) from uni tests (in subfolder).
On a side note, it seems that there are some older test files (shapefiles) that may not be used anymore:
870000_6618000.subset.postIA.las and 870000_6618000.subset.postIA.shp are used in test_application.py |
|
LGTM :) |
Solves #92.
Use yaml to make it easier to copy-paste to a configuration file