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

gdal_calc.py: allow "none" or a float for --NoDataValue #7793

Merged
merged 2 commits into from
May 20, 2023
Merged

gdal_calc.py: allow "none" or a float for --NoDataValue #7793

merged 2 commits into from
May 20, 2023

Conversation

waldner
Copy link
Contributor

@waldner waldner commented May 19, 2023

What does this PR do?

It allows specifying --NoDataValue=none, which was previously failing due to type=float in the parser.add_argument call.

@rouault
Copy link
Member

rouault commented May 19, 2023

would you mind adding in autotest/pyscripts/test_gdal_calc.py a test case for --NoDataValue=none ?

@waldner
Copy link
Contributor Author

waldner commented May 19, 2023

Done, though since I'm not familiar with the test suite I'm not sure exactly what's supposed to be tested for --NoDataValue=none, so any correction/suggestion is welcome.
FWIW, the test seems to be passing (https://github.com/waldner/gdal/actions/runs/5026033430/jobs/9013795215#step:16:14186), but it's entirely possible that it's a coincidence and I'm not performing the right checks.

f"--overwrite --outfile {out[0]}",
)

check_file(out[0], input_checksum[0])
Copy link
Member

Choose a reason for hiding this comment

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

with addition actually checking the nodata value on the output raster like the following (untested) :

Suggested change
check_file(out[0], input_checksum[0])
check_file(out[0], input_checksum[0])
ds = gdal.Open(out[0])
assert ds.GetRasterBand(1).GetNoDataValue() is None

Copy link
Contributor Author

@waldner waldner May 19, 2023

Choose a reason for hiding this comment

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

So perhaps I'll also add another test with --NoDataValue=<actual_float_number> and check that it is also applied, so we can test both none and a real value. Thanks

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What about now?

Copy link
Member

Choose a reason for hiding this comment

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

test_gdal_calc_py_4 actually does -NoDataValue=999. It just lacks the testing of it. Would be better to add it there to minimize the number of lines & runtime

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Shall I add --NoDataValue=none there as well?

Copy link
Member

Choose a reason for hiding this comment

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

Shall I add --NoDataValue=none there as well?

it's probably OK to let it where you added it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, so hopefully now should be it?

@rouault
Copy link
Member

rouault commented May 20, 2023

we have formatting rules that fail with your commit https://github.com/OSGeo/gdal/actions/runs/5028424696/jobs/9019121500?pr=7793
to solve them, run

pip install pre-commit
pre-commit install
pre-commit run --all-files

@waldner
Copy link
Contributor Author

waldner commented May 20, 2023

Oops sorry, done.

@rouault rouault merged commit 283f139 into OSGeo:master May 20, 2023
29 checks passed
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.

None yet

2 participants