Skip to content
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

Reorganize module WorkflowGeoIndicators.groovy #787

Merged
merged 8 commits into from
Dec 2, 2022

Conversation

j3r3m1
Copy link
Collaborator

@j3r3m1 j3r3m1 commented Nov 22, 2022

This PR will reorganize the module WorkflowGeoIndicators in order to simplify the use of individual workflows in the future and to facilitate the use of own RSU by users.

@j3r3m1
Copy link
Collaborator Author

j3r3m1 commented Nov 22, 2022

Remains two main steps which are:

  • replacing part of the code in computeAllGeoIndicators by the corresponding subworkflow
  • isolating the part of the code dedicated to building height estimation in a separate workflow.

Then an other PR would be to leave the possibility to skip the spatial unit creation if the user wants to use its own RSU.

@j3r3m1
Copy link
Collaborator Author

j3r3m1 commented Nov 23, 2022

@ebocher an error is obtained in the test called runBDTopoWorkflowWithSRID(). It seems the parameter snappingDistance passed to the workflow computeAllGeoindicators is null at this stage. I would have expected that in that case the default value for snappingDistance in the workflow computeAllGeoindicators() is taken by default (0.01) but it is not the case, the value is kept as null which cause SQL issue afterward.

What would be the best solution to solve that ? As a simple fix I can modify (add) the value of snappingDistance in the test but you might have a more generic way to solve it ?

@j3r3m1 j3r3m1 mentioned this pull request Nov 23, 2022
@j3r3m1 j3r3m1 marked this pull request as ready for review November 28, 2022 14:17
@j3r3m1 j3r3m1 assigned j3r3m1 and ebocher and unassigned j3r3m1 Nov 28, 2022
Copy link
Member

@ebocher ebocher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What a big change ;-)
Just some comments.
By the way, excellent job.



/**
* Compute the typology indicators
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TEB is not a typology indicators. What about computeRSUIndicators

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This part of the code does not include anything concerning TEB. computeRSUIndicators is not performed here, only typology classifications (UTRF and LCZ) are computed here (based on geoindicators tables)

}

//The spatial relation tables RSU and BLOCK must be filtered to keep only necessary columns
def rsuRelationFiltered = prefix prefixName, "RSU_RELATION_"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure to understand this filter

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rsuRelationFiltered used intead of rsuTable
If yes is rsuTable deleted ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This part of the code was already existing in the previous version. Not sure what contains rsuTable but it might be useless. Yes rsuTable is deleted.

@ebocher ebocher merged commit 6994297 into orbisgis:master Dec 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants