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

Wrong building table used by grid indicators ? #884

Closed
j3r3m1 opened this issue Dec 5, 2023 · 6 comments
Closed

Wrong building table used by grid indicators ? #884

j3r3m1 opened this issue Dec 5, 2023 · 6 comments

Comments

@j3r3m1
Copy link
Collaborator

j3r3m1 commented Dec 5, 2023

It seems the grid indicator calculation is using buildings without the estimated height... It has to be checked. Might be a cache issue ?

@j3r3m1
Copy link
Collaborator Author

j3r3m1 commented Dec 7, 2023

OK, indeed there is a major bug in the OSM workflow. The building table containing the updated height is not used by the grid indicator nor the the noise indicator !!

@ebocher I update the code and you validate ?

I wonder whether it would also be an opportunity to move some parts of the OSM workflow into a more generic functions (in the workflow indicator) instead of having to maintain OSM and BDT workflows. This is particularly the case for the future: if we have a script which makes the indicator calculation for any input data it would be easier if everything is generic (outside the data related workflows).

@j3r3m1
Copy link
Collaborator Author

j3r3m1 commented Dec 7, 2023

OK, indeed there is a major bug in the OSM workflow. The building table containing the updated height is not used by the grid indicator nor the the noise indicator !!

In the osm_processing() in the WorkflowOSM.groovy file, the "buildingTableName" variable is sometimes used while it is updated within the code if the building height estimation is used. Thus "results.building" should be used instead (as it is the case in the "buildingPopulation" function).

@j3r3m1
Copy link
Collaborator Author

j3r3m1 commented Dec 7, 2023

@MelPoupelin do not use the grid indicators as long as this problem is not solved

@ebocher
Copy link
Member

ebocher commented Dec 7, 2023

A mistake certainly due to the numerous reorganizations of the code in recent weeks.
I suggest fixing the table building problem first, before starting a major restructuring that may cause new problems.

@ebocher
Copy link
Member

ebocher commented Dec 8, 2023

I will send a PR to fix that. You've just rescued GeoClimate ;-)

ebocher added a commit to ebocher/geoclimate-1 that referenced this issue Dec 8, 2023
ebocher added a commit that referenced this issue Dec 8, 2023
@ebocher
Copy link
Member

ebocher commented Dec 8, 2023

Fix in #890

@ebocher ebocher closed this as completed Dec 8, 2023
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

No branches or pull requests

2 participants