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

Add 'gdal raster calc' #11850

Merged
merged 9 commits into from
Feb 18, 2025
Merged

Add 'gdal raster calc' #11850

merged 9 commits into from
Feb 18, 2025

Conversation

dbaston
Copy link
Member

@dbaston dbaston commented Feb 13, 2025

What does this PR do?

Adds a gdal raster calc tool that provides much of the functionality of gdal_calc.py. Some differences with gdal_calc.py:

  • VRT output is supported, using the muparser expression engine
  • input names are not restricted to A-Z; the syntax is --input X=file.tif rather than -X file.tif
  • rather than have an --allBands flag, the expression can either refer to all bands of a raster (--calc "X + 3") or a single band of a raster (--calc "X[1] + 3")
  • the --extent union and --extent intersection arguments are not supported, at least for now. I think these may best be handled by separate calls to gdal raster mosaic/stack and gdal raster calc but could be persuaded otherwise without much difficulty
  • the various nodata handling flags of gdal_calc.py are not supported (yet?)
  • there isn't (yet) a good way to use a subset of bands as an input. I'd suggest --input name=dsn[band(s)]
  • Python isn't (yet) available as expression engine

Feedback wanted!

What are related issues/pull requests?

#10067
#11209
#11656

@coveralls
Copy link
Collaborator

coveralls commented Feb 14, 2025

Coverage Status

coverage: 70.034% (-0.005%) from 70.039%
when pulling e07f29e on dbaston:gdal-raster-calc
into 46412f0 on OSGeo:master.

Copy link
Member

@rouault rouault left a comment

Choose a reason for hiding this comment

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

Great work! Just a few comments below

* do not already have such a subscript. For example, "X" would be
* replaced with "X[3]" but "X[1]" would be left untouched.
*/
static std::string SetBandIndices(const std::string &origExpression,
Copy link
Member

Choose a reason for hiding this comment

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

My reading of the implementation is that if someone would use a variable "X" and "aX", and the intented substitution would be only for "X", then "aX" would also get substituted. No ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Line 49 should prevent this. But I'll look at whether I can make test_gdalalg_raster_calc_expression_rewriting less convoluted.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hopefully improved by ed4b07a

int nX{0};
int nY{0};
std::array<double, 6> gt{};
OGRSpatialReference *srs{nullptr};
Copy link
Member

Choose a reason for hiding this comment

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

I assume srs is leaked. The following (or something along that line) should rather be used. (we should probably have a typdef std::unique_ptr<OGRSpatialReference, OGRSpatialReferenceReleaser> OGRSpatialReferencePtr in ogr_spatialref.h)

Suggested change
OGRSpatialReference *srs{nullptr};
std::unique_ptr<OGRSpatialReference, OGRSpatialReferenceReleaser> srs{nullptr};

Copy link
Member

Choose a reason for hiding this comment

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

or maybe just store it as a non-pointer object ?

@rouault rouault merged commit 027f3ca into OSGeo:master Feb 18, 2025
33 of 36 checks passed
@rouault rouault added this to the 3.11.0 milestone Feb 18, 2025
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.

4 participants