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

ensures that "gdal:rasterize" uses integer numbers as input to -ts parameter #60533

Merged
merged 7 commits into from
Feb 12, 2025

Conversation

jakimowb
Copy link
Contributor

Description

fixes #60524
python/plugins/processing/algs/gdal/rasterize.py

rasterize(GdalAlgorithm).getConsoleCommands(...) now ensures that values for the GDAL -ts parameter are written as int values.

@github-actions github-actions bot added this to the 3.42.0 milestone Feb 10, 2025
Copy link

github-actions bot commented Feb 10, 2025

🪟 Windows builds

Download Windows builds of this PR for testing.
Debug symbols for this build are available here.
(Built from commit 8a575a4)

🪟 Windows Qt6 builds

Download Windows Qt6 builds of this PR for testing.
(Built from commit 8a575a4)

@agiudiceandrea
Copy link
Member

agiudiceandrea commented Feb 10, 2025

The previous behaviour was to use floating point values for the -ts parameter, but when the values were used by gdal_rasterize they were truncated (not rounded) internally by GDAL.
The same approach is used now to fix the issue in GDAL OSGeo/gdal#11830.
So I think that, in order to strictly maintain the same behavior, we need to truncate the values (while parameterAsInt rounds the values).
What do you think @elpaso?

@agiudiceandrea agiudiceandrea changed the title ensures that "gda:rasterize" uses integer numbers as input to -ts par… ensures that "gdal:rasterize" uses integer numbers as input to -ts parameter Feb 10, 2025
@jakimowb
Copy link
Contributor Author

@agiudiceandrea
Unfortunately, the type of WIDTH and HEIGHT QgsProcessingParameterNumber widgets cannot be changed depending on
the value of the UNIT QgsProcessingParameterEnum, to Int (UNIT=Pixels) or Float (UNIT=Georeferenced units). This would clearly show users what is expected here.
However, I wasn't aware that parameterAsDouble(..) does rounding. Now I can't think of a case where it would be better to round up instead of truncating. If specified explicitly as PIXELS, it's in responsibility of the user / calling algorithm to ensure the requested transformation to int.

@agiudiceandrea
Copy link
Member

agiudiceandrea commented Feb 12, 2025

@jakimowb, thanks! May you add an explicit test for 'UNITS' : 0, 'WIDTH' : 100.4, 'HEIGHT': 200.6?

@elpaso
Copy link
Contributor

elpaso commented Feb 12, 2025

QgsProcessingParameterNumber

Can you set Qgis::ProcessingNumberParameterType::Integer in the constructor for QgsProcessingParameterNumber ?

@agiudiceandrea
Copy link
Member

agiudiceandrea commented Feb 12, 2025

Can you set Qgis::ProcessingNumberParameterType::Integer in the constructor for QgsProcessingParameterNumber ?

@elpaso, AFAIK it is not possible, since the HEIGHT and WIDTH algorithm's parameters are used for both the -tr option (which needs double values) and the -ts option (which needs integer values).

@jakimowb
Copy link
Contributor Author

@elpaso as @agiudiceandrea mentioned, this is not possible. It would be an interesting new feature if QgsProcessingParameters could be switched on and off or change its configuration depending on other parameter values.

example_width_height_floats

@agiudiceandrea agiudiceandrea added the Processing Relating to QGIS Processing framework or individual Processing algorithms label Feb 12, 2025
@agiudiceandrea agiudiceandrea added the Bug Either a bug report, or a bug fix. Let's hope for the latter! label Feb 12, 2025
@lbartoletti lbartoletti merged commit 8054fd8 into qgis:master Feb 12, 2025
38 checks passed
@qgis-bot
Copy link
Collaborator

The backport to release-3_40 failed:

The process '/usr/bin/git' failed with exit code 128
stderr
error: commit 81c553d0c9cde410290115112d163a94d9a99513 is a merge but no -m option was given.
fatal: cherry-pick failed

stdout
Auto-merging python/plugins/processing/algs/gdal/rasterize.py
[backport-60533-to-release-3_40 e8576ba88a6] ensures that "gda:rasterize" uses integer numbers as input to -ts parameter fixes #60524
 Author: Benjamin Jakimow [email protected] <[email protected]>
 Date: Mon Feb 10 16:18:07 2025 +0100
 2 files changed, 15 insertions(+), 13 deletions(-)

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-release-3_40 release-3_40
# Navigate to the new working tree
cd .worktrees/backport-release-3_40
# Create a new branch
git switch --create backport-60533-to-release-3_40
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick fd61e5cca642c10d3f0e1966a237e912849e485a,81c553d0c9cde410290115112d163a94d9a99513,d9a1f42bd89332bf71ada66d77c859f626009909,ce0c311ba969096f7326cb5df484fe01eb58d819,8d4b6a03dfffa742f2bf1e5b6ca1a0f20f155bbf,244303f1e3064593515f379c901e7aecad150c6d,8a575a422294f054e476dcbdf055c8e1edda515a
# Push it to GitHub
git push --set-upstream origin backport-60533-to-release-3_40
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-release-3_40

Then, create a pull request where the base branch is release-3_40 and the compare/head branch is backport-60533-to-release-3_40.

@qgis-bot qgis-bot added the failed backport The automated backport attempt failed, needs a manual backport label Feb 12, 2025
agiudiceandrea pushed a commit to agiudiceandrea/QGIS that referenced this pull request Feb 12, 2025
@agiudiceandrea
Copy link
Member

Manual backport PR: #60564.

@agiudiceandrea agiudiceandrea removed the failed backport The automated backport attempt failed, needs a manual backport label Feb 12, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport release-3_40 Bug Either a bug report, or a bug fix. Let's hope for the latter! Processing Relating to QGIS Processing framework or individual Processing algorithms
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rasterize (vector to raster) / gdal_rasterize - failed to parse as decimal integer
5 participants