Reduce PostGIS vectors fetching

[strk]

It needs to be profiled but reducing the number of vertices fed to the renderer should speed things up. It's to be profiled to see if we'd be just moving the cost from one end to the other or if it is a general improvement to do so.

[strk]

I'm trying to figure out where to find the pixel size in geographical unite from the postgis datasource, and specifically in "featureset_ptr postgis_datasource::features(const query& q) const". Am I right that "query" doesn't contain this information ? If I have to compute it myself, how can I obtain the output image size ?

[strk]

NOTE: hard-coding the output image size shows me we can run with 0.22 the time it takes now, in certain conditions (1462892 vertices in 2183 geometries becoming 15789 vertices after reduction)

[springmeyer]

sounds like some great progress. In IRC you ask about getting access to the image size. This is not directly accessible. It could be added, but right now instead the resolution is passed down in the query object (this is currently only used by the GDAL plugin for rasters). So, you should be able to use this in the postgis plugin:

double pixel_size_x = 1.0 / boost::get<0>(q.resolution()));
double pixel_size_y = 1.0 / boost::get<1>(q.resolution()));

[strk]

Great, thank you. It indeed gives me the values I'd expect.
Rendering all of italian municipalities (8094) on an 800x600 image shows you can go from an average of 3233.2ms down to an average of 697.17ms using vector reduction. I'm going on with profiling on different zoom levels.

[strk]

You can find an experimental branch on git://github.com/strk/mapnik.git - branch features/pgis_vector_reduction

I would feel better if the snap/simplify calls were decided by XML config file as I can think of cases in which the caller knows they would only add an overhead. What's the best practice for that ?

[strk]

Note that the features/pgis_vector_reduction branch is rooted at 2.0.x, not master

[springmeyer]

NIce work! Being against 2.0.x for now is absolutely fine. Making this user configurable should also be fairly easy. Either the px_gx and px_gh could be made available to the user as tokens in the subselect (like !bbox! and !scale_denominator! are currently) or something like a simply boolean option could be passed as a mapnik::parameter to the datasource constructor like simplify=true or snap=true (any key/value pairs passed to datasources are automatically parsed in xml) Both would need a bit more thought, but those are ideas.

[strk]

Uhm, !scale_denominator! could potentially be enough as long as I can use them in queries. I'm new to mapnik so starting with simple XML file (no subqueries in there)

[strk]

Now that I think about it !scale_denominator! is of no help, we do indeed need px_gx and px_gh, however we want to call them. Would surely need less thoughts as it's just enhancing what's passed around...

The "simplify=true" idea is still nice as a quick way to do w/out thinking too much :)
If you point me to some code to do the parameters passing I'll look into it.

[springmeyer]

Passing user options can be done like:

In XML:

<Datasource>
   <Parameter name="type">postgis</Parameter>
   <Parameter name="simplify">true</Parameter>
</Datasource>

The, in code, you can just do anywhere in the postgis_datasource.cpp:

bool shall_we_simplify = params_.get<mapnik::boolean>("simplify");
if (shall_we_simplify) { /*do something*/ }

[strk]

Great, thanks. Parameter-based simplification pushed to my branch. I can reduce to a single commit if you want. The new commit drops the SnapToGrid call as well as it wasn't that interesting.

[strk]

For the record: it is , not and "true" should be "True" because "false" evaluates to true while "False" evaluates to false.

Should I file another ticket or it's really intended to be hard to set to false and easy to set to true ? (I think anything would evaluate to true)

[strk]

Gah, I was wrong: "True" also evaluates to false !

[springmeyer]

woah? sounds like that might be a regression. mapnik::boolean is a custom type we cooked up to ensure that either True,true, or 1 all evaluate to a c++ true. Can you create a new issue so I can remember to take a look? Thanks!

[strk]

I filed #1141 for the evaluation

[ghost]

strk, for decimation (like you're doing here) ST_Simplify is really overkill - it's doing a lot more work than it needs to. Simplification for decimation can be simply a matter of traversing the coordinate lists and discarding all points within a given tolerance of the previous point. I'm thinking of adding this to JTS, since it's easy to implement.

It might be nice if PostGIS had an ST_Decimate function, to see if that gave a further improvement in performance.

[strk]

On Fri, Mar 23, 2012 at 09:08:26AM -0700, Martin Davis wrote:

strk, for decimation (like you're doing here) ST_Simplify is really overkill - it's doing a lot more work than it needs to. Simplification for decimation can be simply a matter of traversing the coordinate lists and discarding all points within a given tolerance of the previous point.

It would make very dense lines disappear (if every segment is shorter than
your tolerance you discard all of them). The PostGIS version of ST_Simplify
is pretty fast. Not as fast as ST_SnapToGrid but drop more vertices and
the result is much nicer.

I'm thinking of adding this to JTS, since it's easy to implement.

It might be nice if PostGIS had an ST_Decimate function, to see if that gave a further improvement in performance.

Anything in GEOS would incur in the geometry conversion cost.

--strk;

,------o-.
| __/ | Delivering high quality PostGIS 2.0 !
| / 2.0 | http://strk.keybit.net - http://vizzuality.com
`-o------'

[ghost]

Well, you would never delete the start & end point of lines, so the worst that would happen is that lines would be reduced to a single line segment - which not a problem.

If ST_Simplify is fast enough, that's fine.

[strk]

On Sun, Mar 25, 2012 at 08:48:56PM -0700, Martin Davis wrote:

Well, you would never delete the start & end point of lines, so the worst that would happen is that lines would be reduced to a single line segment - which not a problem.

The nice characteristic of DP simplification is that you know the max
distance between the simplified line and the original line will never
be bigger than the given tolerance. You couldn't guarantee that with
the segment-length-based reduction. True that you may get a single line
segment but the input could also be a very large circle composed by tiny
tiny segments...

That said, it's always good to have more options so I'm not against having
one such function to play with.

If ST_Simplify is fast enough, that's fine.

I think it is

--strk;

[strk]

@springmeyer ready to pull again. Let me know if you want me to rebase to 2.0.x

[strk]

uhm, while trying to port the code to 2.0.0-final I found another confusing bug in mapnik::optional.
After setting the parameter I get otput operator return "true" and evaluation evaluating to false. Pretty frustrating:

Here's the code:

            std::clog << "Simplify: " << simplify << std::endl;
            std::clog << "*Simplify: " << *simplify << std::endl;
            std::clog << "Simplify: " << ( simplify ? "yes" : "no" ) << std::endl;

And here's the output:

Simplify:  false
*Simplify: false
Simplify: yes

Puzzling, isn't it ?

[strk]

Alright, I see it's the same in 2.0.x branch. Uff...

I'm fixing this, squashing the multi-commits into a single commit and re-creating the branch after rebasing to 2.0.x

[strk]

Alright 2.0.x-pgis_vector_reduction branch on my repository is now a single commit, rebased to 2.0.x and finally using the optional class in a correct way.

[kunitoki]

interesting stuff, this can be a lot useful. would be cool if we can provide some performance tests for this.

[springmeyer]

Hey @strk - not ignoring this, just traveling on vacation this week. I'd like to play around with this next week and do some perf tests locally before applying. Thanks for your patience.

[strk]

On Wed, Mar 28, 2012 at 07:43:49PM -0700, Dane Springmeyer wrote:

Hey @strk - not ignoring this, just traveling on vacation this week. I'd like to play around with this next week and do some perf tests locally before applying. Thanks for your patience.

No problem. Note that the simplification only happens when the user
requests it via the XML file, so "normal" users won't notice any
unexpected slowdown (other than parsing the additional parameter).

The worst case you'll get is when no vertex is removed, which is
for example on a point layer with many points all falling within
the viewport.

--strk;

,------o-.
| __/ | Delivering high quality PostGIS 2.0 !
| / 2.0 | http://strk.keybit.net - http://vizzuality.com
`-o------'

[ghost]

@strk - a couple of thoughts on your comment about DP preserving the extent of geometries better than decimation.

1. Decimation is really only useful when used with a distance tolerance that is small relative to the output resolution. So if things are "shrunk to a point" it shouldn't matter, since they'd be only visible as a point anyway.
2. It would be possible to add checks to the decimation algorithm such that (one or all) vertices which were on the envelope of the geometry are not removed. This should preserve the overall extent of the geometry.

[strk]

On Thu, Mar 29, 2012 at 07:39:47AM -0700, Martin Davis wrote:

@strk - a couple of thoughts on your comment about DP preserving the extent of geometries better than decimation.

I mentioned preserving shape "look", not extent.

1. Decimation is really only useful when used with a distance tolerance that is small relative to the output resolution. So if things are "shrunk to a point" it shouldn't matter, since they'd be only visible as a point anyway.

I probably don't have a clear definition of decimation.

I guess it would be fine if you always compute "length" from
last "accepted" vertex. I'd have no issue with preserving extent.

Yes, should be faster than ST_Simplify.

--strk;

,------o-.
| __/ | Delivering high quality PostGIS 2.0 !
| / 2.0 | http://strk.keybit.net - http://vizzuality.com
`-o------'

[ghost]

I guess it would be fine if you always compute "length" from
last "accepted" vertex. I'd have no issue with preserving extent.

Yup, that's exactly how decimation works. Nice and simple - should be easy to implement.

[strk]

We ca call this done.

[strk]

@mdavisog (a posteriori notice): your ST_Decimate suggestion could be implemented with http://trac.osgeo.org/postgis/ticket/1137#comment:2