[raster] ST_MapAlgebra2Expr: Map Algebra for 2 rasters

[dustymugs]

Set of function variants for expression-based map-algebra for two rasters.

1. ST_MapAlgebraExpr(

rast1 raster, band1 integer,
rast2 raster, band2 integer,
expression text DEFAULT NULL,
pixeltype text DEFAULT NULL, extenttype text DEFAULT 'INTERSECTION',
nodata1expr text DEFAULT NULL, nodata2expr text DEFAULT NULL, nodatanodataexpr text DEFAULT NULL

)

2. ST_MapAlgebraExpr(

rast1 raster,
rast2 raster,
expression text DEFAULT NULL,
pixeltype text DEFAULT NULL, extenttype text DEFAULT 'INTERSECTION',
nodata1expr text DEFAULT NULL, nodata2expr text DEFAULT NULL, nodatanodataexpr text DEFAULT NULL

)

Should we consider a different function name? Something like ST_MapAlgebra2Expr? Also, is there any reason why we have all the ST_MapAgebraXXX except for distinguishing the type of MapAlgebra?? I think there isn't any issue with calling all of the ST_MapAlgebraXXX functions ST_MapAlgebra due to the different signatures.

The code I've written for this function can definitely be modularized so that there are separate parts, some of which will be done for ST_MapAlgebraFct of 2 raster.

[bnordgren]

I notice that the expression defaults to null. What does that mean? "rast1+rast2"? I think it may make more sense to say "expression text NOT NULL".

PS: What time I spent on PostGIS this past month was spent writing unit tests for the raster iterator/spatial collection framework. If you'd like to kick the tires, it's the "ri-gen2-svn" branch of my postgis repo on github. If the code you mention above is C it should be a relatively simple task to recast it in terms of an Evaluator.

[pracine]

To me the question whether we should give a new name to a function doing essentially the same thing as another one or not is a matter of documentation. Do you want this function to be documented in the same page as her sister one-raster version or not? I think the one and two raster versions should be documented together in the same page and hence have the same name.

We gave different name to the one using an expression (ST_MapalgebraExpr) and to the one using a custom function (ST_MapAlgebraFct) because the mechanism to implement the evaluation expression are very different.

You could use a similar argument to justify a new name for the two raster version...

I think robe should have the last word on this...

[dustymugs]

Replying to bnordgren:

I notice that the expression defaults to null. What does that mean? "rast1+rast2"? I think it may make more sense to say "expression text NOT NULL".

I set "expression" to default to NULL as we don't want to make any assumptions for the end user. Setting the "expression" to "rast1 + rast2" is no better to me than using "1" or "255" as the function doesn't know what the user wants. So, NULL is the default, which means for two pixels of the same coordinates the output pixel's value will be set to NODATA.

[dustymugs]

We gave different name to the one using an expression (ST_MapalgebraExpr) and to the one using a custom function (ST_MapAlgebraFct) because the mechanism to implement the evaluation expression are very different. You could use a similar argument to justify a new name for the two raster version... I think robe should have the last word on this...

Hopefully Regina comments. For now I'll use ST_MapAlgebraExpr. I was using ST_MapAlgebra2Expr...

[bnordgren]

Replying to dustymugs:

I set "expression" to default to NULL as we don't want to make any assumptions for the end user. Setting the "expression" to "rast1 + rast2" is no better to me than using "1" or "255" as the function doesn't know what the user wants. So, NULL is the default, which means for two pixels of the same coordinates the output pixel's value will be set to NODATA.

I think I was unclear. I meant to suggest that because the function cannot make assumptions about what the user wants, it should fail if the user doesn't specify an expression. Besides, failing to specify an expression is not the same thing as specifying an expression which says "RETURN NULL".

In any case, if the user didn't specify an expression, they probably didn't specify a nodata value, so it is highly unlikely that you're going to know enough to be able to set the pixels to NODATA.

Also, I cannot think of a compelling reason to represent the nodata constant value as an expression. Nodata is already defined as a numeric value for the chosen bands, and is a value (not an expression). If we represent it as an expression, it must evaluate to a constant which applies uniformly to all pixels in the band. Let's allow Postgresql to ensure that it does. Users can still write 2+4 if they feel like it.

If we wanted to not fail for lack of information, it seems to me that a minimum set of defaults would be:

...
  expression text DEFAULT "return NULL", ... 
  nodatanodataexpr REAL DEFAULT 0
...

[pracine]

I agree with Bryce:

-expression should be mandatory

-nodatanodataexpr should be a double precision like it should be in the one raster version (see ticket #866)

[dustymugs]

Replying to pracine:

I agree with Bryce: -expression should be mandatory

Done

-nodatanodataexpr should be a double precision like it should be in the one raster version (see ticket #866)

And done.

[pracine]

I used to call it "nodatanodatarepl" for "replacement" as it is not an expression anymore. Don't forget that this specific case does not have to be evaluated anymore by the parser, only assigned.

Would be nice if you could do the same trick to the one raster version too, fixing 866 BTW... Probably David did not have time to do it...

Nice work!

[dustymugs]

I think David did provide a patch in #866 but it never got committed. Can you email David and have make the changes?

I'll use "nodatavaluerepl" to replace "nodatanodataexpr" though I can't say I like "nodatavaluerepl". I think "nodatanodataval" is more expressive.

[pracine]

Replying to dustymugs:

I'll use "nodatavaluerepl" to replace "nodatanodataexpr" though I can't say I like "nodatavaluerepl". I think "nodatanodataval" is more expressive.

No problemo...

[dzwarg]

I had changed the parameter name, but wasn't clear on the code changes at the time. I haven't made any changes regarding the nodatanodataexpr/nodatavaluerepl change, so don't wait up for me!

[dustymugs]

Replying to dzwarg:

I had changed the parameter name, but wasn't clear on the code changes at the time. I haven't made any changes regarding the nodatanodataexpr/nodatavaluerepl change, so don't wait up for me!

I'm adopting nodatanodataval to replace nodatanodataexpr. If you want, I can make the change to the 1-raster mapalgebra.

[dustymugs]

Committed in r8108. Expect refactoring as I work on 2-raster ST_MapAlgebraFct()

[dzwarg]

Replying to dustymugs:

I'm adopting nodatanodataval to replace nodatanodataexpr. If you want, I can make the change to the 1-raster mapalgebra.

I'll be working on Raster on Friday -- if you don't get to it by then, I'll tackle it, no problem.

[dustymugs]

David, no need to worry about it. I had some time last night and made the necessary changes. But please do review as you're far more familiar with the code.