Skip to content

Conversation

@songololo
Copy link
Collaborator

#22

Makes use of f32 for reading, writing, internal arrays.

@biglimp
Copy link
Collaborator

biglimp commented Oct 24, 2025

Hi Gareth, can you please to clarify how these changes affect our code in UMEP for QGIS. As far as we can see, you have changed e.g. solweig_run.py. Have you tested this in QGIS environment? I think maybe we have missed this from a previous PR. Why are you making changes in dailyshading.py as this is not used in SOLWEIG? Please clarify?

@songololo
Copy link
Collaborator Author

songololo commented Oct 27, 2025

Hi @biglimp - for this pull request I tried to change all instances of saving rasters to use f32 by default. I searched for all instances of common.save_raster lines where I added coerce_f64_to_f32=True which is a new setting to cast f64 arrays to f32 when saving. If preferred these can be set to False to leave as f64.

The changes to dailyshading.py were to the common.save_raster lines. When saving the file on my vscode it auto formats using the recommended ruff formatter, so the other changed lines are to do with formatting; e.g. ((vegdem2 * vegdem)) became (vegdem2 * vegdem) and int(0) became 0. It also automatically wraps or unwraps lines according to the formatting spec.

The changes to Solweig_run.py are similar (coerce_f64_to_f32=True).

What would be good is to find a way to test the common.py file using some automatic tests since this file needs to automatically detect whether it is running with access to rasterio or not, in which case we assume it is using gdal from QGIS. I'm not sure how to simulate a QGIS environment for testing but if anyone has ideas it would be nice to get some tests setup so that they automatically run when creating a pull request.

Another nice change would be to separate old code into a separate directory and to then auto format all current code using ruff which will help catch some potential errors like the f-string errors that pop up from time to time.

@biglimp
Copy link
Collaborator

biglimp commented Oct 28, 2025

Hi. Thanks for this info. I think you point towards the issue that we cannot control. There is no good way to test within a QGIS environment without doing it manually.

On another note, @nilswallenberg and I are discussing to create a separate pypi for the solweig model consisting of only the model itself, without any other workflow tools such as SVF-calculator. Then we can all use that pypi module to run the actual model. Do you see any issues with this?

@songololo
Copy link
Collaborator Author

@biglimp apologies for the slow response.

At the moment umep-core is designed to work either from Python or from QGIS (not tested at this point).

For this to work, the common.py module is intended to check whether running in QGIS or not and to execute reading and writing operations accordingly (GDAL vs rasterio).

Personally I would recommend to keep SOLWEIG and other core functions for SVF and shadows together so that it isn't necessary to install and manage multiple packages. Though it would be easier to maintain if old or dead code is removed (or moved to a deprecated folder).

@biglimp
Copy link
Collaborator

biglimp commented Nov 24, 2025

@biglimp apologies for the slow response.

At the moment umep-core is designed to work either from Python or from QGIS (not tested at this point).

For this to work, the common.py module is intended to check whether running in QGIS or not and to execute reading and writing operations accordingly (GDAL vs rasterio).

Personally I would recommend to keep SOLWEIG and other core functions for SVF and shadows together so that it isn't necessary to install and manage multiple packages. Though it would be easier to maintain if old or dead code is removed (or moved to a deprecated folder).

Sorry for late reply. I see your point but nevertheless, SVF and wall algorithm are separate tools that users make use of for many other applications a part from SOLWEIG-calculation. SOLWEIG is a separate model and therefore, it would be better to maintain this separately.

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.

3 participants