[raster]: st_intersects signature change - documentation out of synch

[robe]

I noticed my raster comments install is failing. It seems all the ST_Intersects that take the exclude_nodata_value have been removed.

​http://www.postgis.org/documentation/manual-svn/RT_ST_Intersects.html

Is that intentional? Just wanted to verify before I remove them from the documentation.

[dustymugs]

Yes. In the comments of ticket #1176, it was the opinion that providing nband implies that the user wants a specific intersection test. If nband is not specified, only the convex hull of the raster is tested against the geometry. If nband is specified, the geometry is converted to a raster and then the two rasters are compared.

[robe]

On other comment I see we have

ST_Intersects(rastA, rastB, nbanda,nbadb);

and

ST_Intersects(rastA,nbanda, rastB, nbandb)

Can we PLEASE get rid of one of these. I for one don't know the difference and don't feel like explaining this to anyone when they ask which they should use.

I would keep (1) which I have already documented and get rid of (2).

Also I see you changed ST_AsRaster signature which broke my extension install. I'm all fine with that, but I think its time to start working on an upgrade minor script since people have started using raster functionality and I'm sure they don't feel like reloading gigs of data.

I'll take a stab at this if you guys would like first by dropping all the functions you have removed so they don't cause name conflicts.

[dustymugs]

I've removed (2) as per your suggestion. Associated revision is r7901.

I think all other functions and their variants are stable and shouldn't undergo any further changes. I added the "touched" option to ST_AsRaster as I considered using it for ST_Intersects in addition to exposing the option to end-users.

[pracine]

Normally the band number should follow the raster parameter… I don't think we made it a rule but we should.

[dustymugs]

Regina and Pierre,

Can you two decide whether or not keep the ST_Intersects(raster, int, raster, int) variation and let me know? I see the pros and cons of both arguments but have no particular preference. When I've used ST_Intersects for my work, I've used both depending how lazy I was.

As for the regression test failure Regina, I'll act upon that once Pierre and you decide on the fate of ST_Intersects(raster, int, raster, int).

[robe]

Pierre should have the final word on this. Just my 2c.

I just think the number of variants you have is getting out of hand. It's worse when you have two functions that take exactly the same arguments but in different order. Too prone for ambiguous name references or when things accidentally get auto cast.

The only thing going for raster,int,raster,int is that it follows the pattern of other functions (rast and then band). The downside of it is that if someone just wanted to to rast, rast — then they would have to input all args. This is why I preferred

rast1,rast2,int1,int2

over rast1, int1, rast2, int2

the number of permutations of these while may make some things shorter to type I think will just cause to confuse new users and old users and become a great burden to maintain as you'd have to think of all cases where things may be thrown in the wrong slot.

I know Pierre loves overloading because it gives users the freedom to not remember the order of things and arbitrarily leave things out willy nilly, I just think it adds too much management burden and scares users off when they see a function with 12 overloads.

[pracine]

We maybe don't have to document all the overloads and just says, in general for the specific case of band number: "Band numbers can be omitted. In this case they default to 1." This solve your management burden and keeps a certain rule about parameter's order.

Still I think we need a written set of rules for parameters order and defaults. This could be added at the beginning of the reference section.

I still prefer (rast1, int1, rast2, int2) over (rast1, rast2, int1, int2).

PostGIS never had this problem of two many overloads because the internal structure of the object is more simple than raster. In our case many functions must get many parameters…

[robe]

How about this

Make

rast1,rast2,nband1,nband2

just

rast1,rast2

Don't allow people to use bands with that version.

It doesn't really solve my management burden, though. I'm more concerned about the permutations and the possibility of text cast to integer and integer to text and that kind of stuff which ends up making functions go the wrong path than what the user intended.

[pracine]

Replying to robe:

How about this

Make

rast1,rast2,nband1,nband2

just

rast1,rast2

Don't allow people to use bands with that version.

This would be inconsistent with ST_Intersection(raster1, band1, raster2, band2) which is planned to work on single bands. (If not, what would be the result of ST_Intersection(raster1, raster2) if they both have multiple bands? This is the big question of the day!)

It doesn't really solve my management burden, though. I'm more concerned about the permutations and the possibility of text cast to integer and integer to text and that kind of stuff which ends up making functions go the wrong path than what the user intended.

It seems to me that those casting problems should not force us to adopt one signature over another one. Are we slave here or masters?

[robe]

We are slaves when debugging and masters when coding. The more permutations you have the harder it is to determine what went wrong so you must balance your slave/master duality.

I'm missing your point here Pierre

I'm saying keep the rast1,nband1,rast2,nband2

But the other one: get rid of nband1 and nband2 at the end of it. It's not even consisten with your intersection and violates your convention.

I thought you assumed that if no band is specified then band1 is assumed

So ST_Intersection(raster1,raster2) —- means first band of both.

But of course I'm pretty fine with your getting rid of that, though will probably break some code.

[dustymugs]

I like Regina's compromise of turning ST_Intersects(raster, raster, int, int) into ST_Intersects(raster, raster) with the integers assumed to be 1 and 1. This is consistent with other raster functions.

[pracine]

Replying to dustymugs:

I like Regina's compromise of turning ST_Intersects(raster, raster, int, int) into ST_Intersects(raster, raster) with the integers assumed to be 1 and 1. This is consistent with other raster functions.

Yes. This is the default. But what if I want to intersect band 2 of raster 1 with band 3 of raster 2?

At the beginning there was this discussion about forcing users to use ST_Band() to select a particular band… We choose to add a band parameter to every function instead. That would be a solution if there is no cost but what a change for just a couple of variants.

I guess this is the first function taking two raster band. Right? This is why we are puzzled so much.

On another side: Why is there two versions of _st_intersects(rast raster, geom geometry, nband integer DEFAULT NULL) in rtpostgis.sql.in.c?

[dustymugs]

Replying to pracine:

Replying to dustymugs:

I like Regina's compromise of turning ST_Intersects(raster, raster, int, int) into ST_Intersects(raster, raster) with the integers assumed to be 1 and 1. This is consistent with other raster functions.

Yes. This is the default. But what if I want to intersect band 2 of raster 1 with band 3 of raster 2?

ST_Intersects(raster1, raster1band2, raster2, raster2band3)

At the beginning there was this discussion about forcing users to use ST_Band() to select a particular band… We choose to add a band parameter to every function instead. That would be a solution if there is no cost but what a change for just a couple of variants.

We don't need ST_Band() and I don't see any mention of ST_Band outside of this…

I guess this is the first function taking two raster band. Right? This is why we are puzzled so much.

On another side: Why is there two versions of _st_intersects(rast raster, geom geometry, nband integer DEFAULT NULL) in rtpostgis.sql.in.c?

I kept the old raster converted to geometry functionality due to edge cases where a geometry-geometry intersects test is true while a raster-geometry intersects set is false. Check the Working specs for a visual as I detailed out why I kept it.

So, if ST_Intersects is called with the first argument being a raster…

ST_Intersects(raster, geometry)

the geometry is converted to raster and a raster-raster test ensues

If ST_Intersects is called with a geometry as the first argument…

ST_Intersects(geometry, raster)

the raster is converted to a geometry and a geometry-geometry test occurs.

Definitely check the working specs.

[pracine]

Replying to dustymugs:

Replying to pracine:

Replying to dustymugs:

I like Regina's compromise of turning ST_Intersects(raster, raster, int, int) into ST_Intersects(raster, raster) with the integers assumed to be 1 and 1. This is consistent with other raster functions.

Yes. This is the default. But what if I want to intersect band 2 of raster 1 with band 3 of raster 2?

ST_Intersects(raster1, raster1band2, raster2, raster2band3)

At the beginning there was this discussion about forcing users to use ST_Band() to select a particular band… We choose to add a band parameter to every function instead. That would be a solution if there is no cost but what a change for just a couple of variants.

We don't need ST_Band() and I don't see any mention of ST_Band outside of this…

I guess this is the first function taking two raster band. Right? This is why we are puzzled so much.

On another side: Why is there two versions of _st_intersects(rast raster, geom geometry, nband integer DEFAULT NULL) in rtpostgis.sql.in.c?

I kept the old raster converted to geometry functionality due to edge cases where a geometry-geometry intersects test is true while a raster-geometry intersects set is false. Check the Working specs for a visual as I detailed out why I kept it.

So, if ST_Intersects is called with the first argument being a raster…

ST_Intersects(raster, geometry)

the geometry is converted to raster and a raster-raster test ensues

If ST_Intersects is called with a geometry as the first argument…

ST_Intersects(geometry, raster)

the raster is converted to a geometry and a geometry-geometry test occurs.

Definitely check the working specs.

Does that mean that ST_Intersects(geometry, raster) is not commutative anymore? Why touching ST_Intersects(geometry, raster)? Weren't the specs just for ST_Intersects(raster, raster)?

[robe]

So we are in agreement? The signatures will be

change: ST_Intersects(rast1,nband1,rast2,nband2) → ST_Intersects(rast1,rast2) Put back: ST_Intersects(rast1,nband1,rast2,nband2)

This keeps consistency and avoids the situation where someone throws in an untyped NULL and we then get an ambiguous path like this:

ST_Intersects(rast,NULL,NULL,2) ;

I'm almost tempted to say just create a new function name — call this RT_Intersects for raster space intersection (which means operation done in raster space)

and RT_Intersects(raster,geometry) RT_Intersects(raster,raster) RT_Intersects(raster1,band1,raster2,band2)

The issue I have with this: ST_Intersects(raster, geometry)

ST_Intersects(geometry, raster)

As Pierre pointed out is that it is no longer commutative and people will assume it is when clearly you are saying one is done in raster space and one is done in vector space.

[pracine]

I would suggest anything done in raster space (i.e. raster with raster or geometries converted to raster before processing) like ST_Intersects(0, ST_Intersection(), ST_Union(), etc… to be named ST_IntersectsAsRaster(), ST_IntersectionAsRaster(), ST_UnionAsRaster() or so it is clear what is happening inside and they still stick to their geometry sister functions. This brake a bit the idea of seemless operators but I think we have no choice.

[robe]

I'm in agreement. I like those names.

[dustymugs]

I'd rather see us keep the seamless operators at the expense of being commutative. Having ST_IntersectsAsRaster and others like it will get really old and really fast. I think users would prefer to learn just one function name and then switch in and out geometries and rasters. I know I do.

This will also affect:

ST_Within

ST_Contains

ST_Overlaps

ST_Difference

ST_SymDifference

ST_Equals

ST_Disjoint

ST_Touches

ST_Crosses

ST_Covers

ST_IsCoveredBy

ST_Relate

[pracine]

Another solution I thought about was to signal the fact that everything was processed as raster by passing in a third argument 'RASTER' saying "this must be processed as raster and must return a raster"…

I think we will have to formulate two or three solutions and get everybody to vote (Jorge, David, Mateusz, Sandro, Bryce?)… Damned open source democracy

[robe]

I'm indifferent

[dustymugs]

Replying to robe:

So we are in agreement? The signatures will be

change: ST_Intersects(rast1,nband1,rast2,nband2) → ST_Intersects(rast1,rast2) Put back: ST_Intersects(rast1,nband1,rast2,nband2)

This keeps consistency and avoids the situation where someone throws in an untyped NULL and we then get an ambiguous path like this:

ST_Intersects(rast,NULL,NULL,2) ;

+1 for this. If Pierre consents, I'll make the changes.

[dustymugs]

Replying to pracine:

Another solution I thought about was to signal the fact that everything was processed as raster by passing in a third argument 'RASTER' saying "this must be processed as raster and must return a raster"…

I think we will have to formulate two or three solutions and get everybody to vote (Jorge, David, Mateusz, Sandro, Bryce?)… Damned open source democracy

If you're going to get everyone to vote, I'd ask everyone on postgis-devel as I'd like Paul, Mark and Sandro to have a say.

Personally, I'm for keeping the seamless API at the expense of being commutative and fully against additional function parameters.

[pracine]

Replying to dustymugs:

Replying to robe:

So we are in agreement? The signatures will be

change: ST_Intersects(rast1,nband1,rast2,nband2) → ST_Intersects(rast1,rast2) Put back: ST_Intersects(rast1,nband1,rast2,nband2)

This keeps consistency and avoids the situation where someone throws in an untyped NULL and we then get an ambiguous path like this:

ST_Intersects(rast,NULL,NULL,2) ;

+1 for this. If Pierre consents, I'll make the changes.

+1

[pracine]

Replying to dustymugs:

Replying to pracine:

Another solution I thought about was to signal the fact that everything was processed as raster by passing in a third argument 'RASTER' saying "this must be processed as raster and must return a raster"…

I think we will have to formulate two or three solutions and get everybody to vote (Jorge, David, Mateusz, Sandro, Bryce?)… Damned open source democracy

If you're going to get everyone to vote, I'd ask everyone on postgis-devel as I'd like Paul, Mark and Sandro to have a say.

Personally, I'm for keeping the seamless API at the expense of being commutative and fully against additional function parameters.

I'm pretty sure overloading does not take the return type into account so you will have to do something anyway otherwise ST_Intersection(raster, geometry) → raster will conflict with ST_Intersection(raster, geometry) → set of geomval. Right Robe?

[dustymugs]

I'm pretty sure overloading does not take the return type into account so you will have to do something anyway otherwise ST_Intersection(raster, geometry) → raster will conflict with ST_Intersection(raster, geometry) → set of geomval. Right Robe?

What I was thinking was:

ST_Intersection(raster, geometry) → raster

ST_Intersection(geometry, raster) → set of geomval

[dustymugs]

Signature changed for ST_Intersects(raster, raster, int, int) → ST_Intersects(raster, raster) in r7919.

[pracine]

Replying to dustymugs:

I'm pretty sure overloading does not take the return type into account so you will have to do something anyway otherwise ST_Intersection(raster, geometry) → raster will conflict with ST_Intersection(raster, geometry) → set of geomval. Right Robe?

What I was thinking was:

ST_Intersection(raster, geometry) → raster

ST_Intersection(geometry, raster) → set of geomval

Ha yeah… If Regina is ok with that, +1 for me… We will have to be explicit in the reference though. I predict many mails noting the strange incongruency.

[robe]

Yah fine with me. It's a bit weird but then again geomval isn't a geometry either so stranger things have happened.

[pracine]

BTW this choice implies that we have to do the same with ST_Intersection, ST_Difference, ST_SymDifference, ST_Union, ST_Within, ST_Contains, ST_Overlaps, ST_Equals, ST_Disjoint, ST_Touches, ST_Crosses, ST_Covers, ST_IsCoveredBy and ST_Relate: every time the raster is first, everything is converted and processed in raster mode. When the geometry is first, everything is converted to vector and processed as vector. Are we all understanding the same thing?

[dustymugs]

Yes. I understand completely. This is what I anticipated. As long as we're consistent and users understand how it is works for one function, they should be able to just apply it to the rest.

[robe]

Now if only I can keep track of the difference between the two sides of the function in memory.