-
Notifications
You must be signed in to change notification settings - Fork 25
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Update build boundaries #418
base: master
Are you sure you want to change the base?
Conversation
Se esta trabajando en la implementación del nuevo algoritmo de construcción de linderos, en un principio se desea conservar el comportamiento que tenia el algoritmo existente en el cual se le permitia al usuario realizar una selección de linderos antes de proceder a construir los linderos. Realizar la selección de linderos tiene la ventaja de que si el conjunto de datos es muy grande no es necesario correr todo el algoritmo para toda la base de datos sino unicamente para los linderos seleccionados. Parece una buena estragería, el problema que actualmente existe al adoptarla es que al no seleccionar todos los linderos que estan conectados el resultado obtenido no es el esperado. Por ejemplo, se puede apreciar en el siguiente gif que al generar los linderos para la zona seleccionada el total de linderos es 58602. Pero si lo genero para toda la base de datos el total de linderos es 58613. A partir de los linderos seleccionados por el usuario se realiza una selección por localización y se seleccionan los linderos que estan conectados pero puede ocurrir lo siguiente: los linderos seleccionados no incluyen todos los linderos vecinos razón por la cual la construcción no es la correcta. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we rebase this PR (on lev_cat_1_1) to make it ready to be reviewed/merged?
That would also show us the unit tests status.
Procedere con la actualización 👍 |
e5769c6
to
fb6dbeb
Compare
This PR was rebase using lev_cat_1_1. Now this one is ready to be reviewed/merged. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We've never used [feat]
in our commits. For your information, inside the square brackets we are get used to add the module name.
fb6dbeb
to
392b326
Compare
4b90445
to
7830fec
Compare
I'm getting errors on GNU/Linux, using QGIS v3.16.5 and master: The result of the Build Boundaries model is a layer which has its field names truncated, like in the image below: Because of that, the ETL cannot run well, and I get errors like the following in the console:
We cannot merge until we know what's happening and fix it. This should be done on top of the latest commit (7830fec). Testing on your latest commit (392b326) gave me the same results. |
Apart from that, it would be required to have a test for the toolbar's method called
Ideally the test should act in two modes: using a selection and using all the features. |
Thank you very much for the review and adjustments made. I will proceed to review the observations. I would like to confirm if the dataset with which the error was generated corresponds to the structured data for the tutorial? |
Yes, but migrated to Levantamiento Catastral v1.1. |
Build_Boundaries model was updated to prevent truncated data. |
I have designed the test to validate the entire Do you think it is a good idea to validate the whole https://gist.github.com/lacardonap/157b1aa7f36b0e178758d2f00a3e75e5 I am getting this error when running the test:
Any advice is welcome. Thanks |
Do you think it's a good idea to delete features from the user layer and do not try to recover those deleted features if any failure occurs while running the model? Could you commit your test to another branch (created from |
I appreciate the support in reviewing the test which was left in the branch https://github.com/SwissTierrasColombia/Asistente-LADM-COL/tree/test_toolbar_build_boundaries, which never ever worked for me locally. I am attentive to what is required. |
53a8209
to
0f722d1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test test_build_boundaries
does not fail anymore due to processing plugin. But it's throwing other errors now, that I'll pass for you to adjust.
Additionally, the toolbar.build_boundary()
method should be adjusted:
- Don't delete features until you know you have obtained the new features from the processing model.
- When deleting features, keep them in a variable because if the insertion of new features fails, you should restore the deleted features. The user should never end up with an incomplete data set (which you can see in https://user-images.githubusercontent.com/652785/120557677-e7cddf00-c3c3-11eb-8800-44f61e778a07.png).
Thank you for your support. I continue with the requested adjustments. |
The requested adjustments are made. Please continue with the review. I stay tuned for what is required |
asistente_ladm_col/gui/toolbar.py
Outdated
@@ -118,7 +118,7 @@ def build_boundary(self, db): | |||
layers[db.names.LC_BOUNDARY_T].deleteSelectedFeatures() | |||
|
|||
# Bring back the features we deleted before, but this time, with the boundaries fixed | |||
self.app.core.run_etl_model_in_backgroud_mode(db, build_boundaries_layer, db.names.LC_BOUNDARY_T) | |||
self.app.core.run_etl_model_in_backgroud_mode(db, build_boundaries_layer, db.names.LC_BOUNDARY_T, False) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If something has to change, it has to be the call from the tests, not the one from core...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you very much for the feedback. The reply is late but I prefer to respond.
You are right, the suggested adjustment was made and currently in the commit 513f31a
asistente_ladm_col/gui/toolbar.py
Outdated
# Bring back the features we deleted before, but this time, with the boundaries fixed | ||
self.app.core.run_etl_model_in_backgroud_mode(db, build_boundaries_layer, db.names.LC_BOUNDARY_T, False) | ||
# Build boundaries should have generated at least one boundary. | ||
if selected_boundaries_count > 0 and build_boundaries_count > 0: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And what if not? Any message for users?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adjustment made, available in commit 513f31a
@@ -1152,7 +1152,7 @@ def copy_csv_to_db(self, csv_layer, db, target_layer_name): | |||
return True | |||
|
|||
@_activate_processing_plugin | |||
def run_etl_model_in_backgroud_mode(self, db, input_layer, ladm_col_layer_name): | |||
def run_etl_model_in_backgroud_mode(self, db, input_layer, ladm_col_layer_name, force_reprojection=True): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't get it. If you need to adjust something here is to set the proper CRS to your layer before calling this method. The method run_etl_model_in_backgroud_mode
can remain without changes.
Is there any problem trying to set a proper CRS to your layer?
You should use get_ctm12_crs()
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right, the suggested adjustment was made and currently in the commit 513f31a
asistente_ladm_col/gui/toolbar.py
Outdated
layers[db.names.LC_BOUNDARY_T].dataProvider().truncate() | ||
|
||
# Original data is restored because an error occurred while trying to build the boundaries | ||
self.app.core.run_etl_model_in_backgroud_mode(db, copy_boundary_layer, db.names.LC_BOUNDARY_T, False) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just restore selected features, no need to do it for the whole layer...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wooow thanks for the comment, you are absolutely right, adjustment made in the commit 513f31a
asistente_ladm_col/gui/toolbar.py
Outdated
with edit(layers[db.names.LC_BOUNDARY_T]): | ||
# Delete selected features as they will be imported again from a newly created layer after processed | ||
layers[db.names.LC_BOUNDARY_T].deleteSelectedFeatures() | ||
copy_boundary_layer = self.app.core.get_layer_copy(layers[db.names.LC_BOUNDARY_T]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I disagree. Imagine a layer of 1000 boundaries and you select 4 of them before clicking on "Build Boundaries"...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adjustment made, available in commit 513f31a
asistente_ladm_col/gui/toolbar.py
Outdated
# Bring back the features we deleted before, but this time, with the boundaries fixed | ||
self.app.core.run_etl_model_in_backgroud_mode(db, build_boundaries_layer, db.names.LC_BOUNDARY_T, False) | ||
|
||
# check if features were insert successful |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
successfully
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adjustment made, available in commit 513f31a
asistente_ladm_col/gui/toolbar.py
Outdated
|
||
else: | ||
# Clean layer because wrong data could be insert previously | ||
layers[db.names.LC_BOUNDARY_T].dataProvider().truncate() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're removing all features?
What if the user just selected 1% of them?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the change was made, now only the selected boundaries are deleted and if an error occurs they are restored. Available in commit 513f31a
asistente_ladm_col/gui/toolbar.py
Outdated
"{} feature(s) was(were) analyzed generating {} boundary(ies)!").format(num_boundaries, build_boundaries_layer.featureCount())) | ||
|
||
else: | ||
# Clean layer because wrong data could be insert previously |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could have been inserted previously
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adjustment made, available in commit 513f31a
'Features count does not match for the layer {}'.format(layer_name)) | ||
|
||
def test_build_boundaries_with_empty_geom(self): | ||
print('\nINFO: Validating build boundaries with features with null geometry...') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unclear what you're testing.
What is the result you expect? The NULL geometry is ignored?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the information given in the tests has been updated. This test validates that if there is a boundary without geometry it must be removed. vailable in commit 513f31a
'Null features count does not match for the layer boundary layer') | ||
|
||
boundary_layer.selectAll() | ||
self.toolbar.build_boundary(db_gpkg) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The method build_boundaries should be called like this:
build_boundary(db_gpkg, False) # where False is do not use selection
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adjustment made, available in commit 513f31a
We are wasting too much time in this review. Please double (and triple) check before submitting to a review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- The test is not passing.
- To keep the commit history clean, squash all commits since 34a5e0f on.
asistente_ladm_col/gui/toolbar.py
Outdated
@@ -45,7 +45,14 @@ def __init__(self, iface): | |||
self.app = AppInterface() | |||
self.geometry = GeometryUtils() | |||
|
|||
def build_boundary(self, db): | |||
def build_boundary(self, db, use_user_selection=True): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- False by default, since the expected and recommended way to run this method is without a selection.
- Change
use_user_selection
byuse_selection
like in other methods in the same class and like in GeometryUtils().
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Change
use_user_selection
byuse_selection
like in other methods in the same class and like in GeometryUtils().
The build_boundary method is designed to display a dialog box to the user indicating that no selection has been made and asking if the user wants to use all boundaries. When there are selected boundaries, no dialog box is shown and the selected boundaries are used. The purpose of using the parameter use_user_selection in the function is to omit the user selection and use all the records of the layer.
It is proposed to change use_user_selection to skip_selection where:
skip_selection (Boolean): True if we omit the boundaries selected by the user, False if we validate the user's selection.
asistente_ladm_col/gui/toolbar.py
Outdated
@@ -58,6 +65,9 @@ def build_boundary(self, db): | |||
} | |||
self.app.core.get_layers(db, layers, load=True) | |||
|
|||
if not use_user_selection: | |||
layers[db.names.LC_BOUNDARY_T].selectAll() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't seem to make sense.
If use_selection is False, you shouldn't alter selected layer features nor use them in any way. Instead you should use always all layer features (e.g., when creating a copy, when counting features, etc.).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right, the suggested adjustment was made and currently in the commit 513f31a
asistente_ladm_col/gui/toolbar.py
Outdated
layers[db.names.LC_BOUNDARY_T].dataProvider().truncate() | ||
if layers[db.names.LC_BOUNDARY_T].featureCount() != boundaries_count - selected_boundaries_count: | ||
# Clean layer because wrong data could have been inserted previously | ||
expr = "{} IN ('{}')".format(db.names.T_ILI_TID_F, "','".join([str(t_ili_tid) for t_ili_tid in boundary_t_ili_tids])) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if you actually need str(t_ili_tid)
, since t_ili_tids are already strings.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
data type of t_ili_tid
is string, no conversion is necessary. Adjustment made, available in commit 513f31a
asistente_ladm_col/gui/toolbar.py
Outdated
# Clean layer because wrong data could have been inserted previously | ||
expr = "{} IN ('{}')".format(db.names.T_ILI_TID_F, "','".join([str(t_ili_tid) for t_ili_tid in boundary_t_ili_tids])) | ||
layers[db.names.LC_BOUNDARY_T].selectByExpression(expr) | ||
layers[db.names.LC_BOUNDARY_T].invertSelection() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you need to select and then invert the selection, then the expression is wrong.
Perhaps you need expr = "{} NOT IN ('{}')"...
, don't you?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the observation, the selection is much more optimal. Adjustment made, available in commit 513f31a
|
||
# Original data is restored because an error occurred while trying to build the boundaries | ||
self.app.core.run_etl_model_in_backgroud_mode(db, copy_boundary_layer, db.names.LC_BOUNDARY_T, False) | ||
# the previously deleted boundaries are restored because an error occurred when trying to insert the building boundaries |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any chance to write a unit test for that case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am trying to generate a data set that will allow us to test this part of the cosw. It has not been possible, but I would also like to have a test that checks this.
asistente_ladm_col/lib/geometry.py
Outdated
return processing.run("model:Build_Boundaries", params)['native:refactorfields_2:built_boundaries'] | ||
build_boundary_layer = processing.run("model:Build_Boundaries", params)['native:refactorfields_2:built_boundaries'] | ||
|
||
# For CTM12 the output layer remains without projection. The projection of the input layer is assigned |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it necessary?
Wasn't it only an issue for unit tests?
If you really need to set the CRS here, just use layer.setCrs()
.
@@ -105,8 +105,7 @@ def test_build_boundaries_with_empty_geom(self): | |||
self.assertEqual(boundary_layer.selectedFeatureCount(), 1, | |||
'Null features count does not match for the layer boundary layer') | |||
|
|||
boundary_layer.selectAll() | |||
self.toolbar.build_boundary(db_gpkg) | |||
self.toolbar.build_boundary(db_gpkg, False) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This call should be simply self.toolbar.build_boundary(db_gpkg)
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adjustment made, available in commit 513f31a
asistente_ladm_col/gui/toolbar.py
Outdated
@@ -45,7 +45,14 @@ def __init__(self, iface): | |||
self.app = AppInterface() | |||
self.geometry = GeometryUtils() | |||
|
|||
def build_boundary(self, db): | |||
def build_boundary(self, db, use_user_selection=True): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can rename the method to build_boundaries()
, since that's what the method does.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adjustment made, available in commit 513f31a
@@ -22,6 +22,7 @@ def setUpClass(cls): | |||
import_qgis_model_baker() | |||
import_asistente_ladm_col() # Import plugin | |||
cls.app = AppInterface() | |||
cls.app.core.initialize_ctm12() # We need to initialize CTM12 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if you initialize CTM12 after this line instead (and remove any other call to such method in unit tests)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What a good idea, adjustment made. initialize CTRM12 from other classes were removed. Available in commit 513f31a
@gacarrillor thank you very much for the review, I am checking why if I run only the toolbar tests it works correctly but if I run all the tests it fails. I will proceed with the requested adjustments |
The problem was solved by moving the test |
I am currently trying to generate test data to extend the test coverage. The last commit will correspond to the squash all commits since 34a5e0f on. Again, thanks for the review. |
3609a57
to
1adde0c
Compare
In trying to generate a dataset that would allow to extend the coverage of the tests, some improvements were identified and implemented and the corresponding tests were developed. The suggested adjustments are already implemented but to date I have not been able to generate a test data set that fails (the grass algorithm is very good). You can now proceed with the revision. Thanks |
…ng the former inefficient method; add tests
… the build_boundaries function and include tests to check build boundaries function with invalid data
1adde0c
to
a6ea115
Compare
4755316
to
6791d59
Compare
Actualización del algoritmo que permite construir los linderos
model_build_boundaries.model3