search

3liz / qgis-pgmetadata-plugin

QGIS Plugin to manage some metadata from PostgreSQL layer
GNU General Public License v2.0
12 stars 10 forks source link

Add raster support #104

Closed effjot closed 2 years ago

effjot commented 2 years ago

I’ve added basic raster support:

effjot commented 2 years ago

Sorry, I’ve overlooked that some tests have failed. I’ll update the PR tomorrow.

Gustry commented 2 years ago

No worries for the test ! Thanks a lot for this nice contribution !

For isort and flake8, you can have a look https://github.com/3liz/qgis-pgmetadata-plugin/blob/master/CONTRIBUTING.md#code

Let me know if you need assistance with these tests ;-)

effjot commented 2 years ago

When I prepared the PR, I had to separate raster support from other custom additions (e.g. button for adding theme layers; database field for license attribution) and obviously broke adding the dates in upgrading the trigger function calculate_fields_from_data(). I hope everything’s alright now.

At home, I use Debian stable which comes with Qgis 3.10 instead of 3.16/3.22 which I use at work. It seems raster support is limited in Qgis 3.10 – you can load a raster layer from the database, but the metadata dock doesn’t recognise it as being stored in PostgreSQL. Adding via locator search crashes Qgis 3.10; judging from the “table” icon in the locator search results, the layer isn’t correctly recognised as a raster. Qgis 3.16 and higher work as expected.

effjot commented 2 years ago

Should I try to write a test for raster data, similar to DatabaseTestCase in test/base_database.py?

Gustry commented 2 years ago

For the test, it's up to you :

I think in test_sql.py is ok.

Let me know if you need some help.

effjot commented 2 years ago

Thanks for the pointers. However, I won‘t be able to write the tests in the next few days.

effjot commented 2 years ago

I also realised that with this PR, the plugin doesn’t work with databases without the postgis_raster extension anymore, because then the table raster_columns doesn’t exist. I’ll have to update the code of pgmetadata.calculate_fields_from_data() accordingly.

Gustry commented 2 years ago

Nice, it seems you have added tests etc :) and it's green. Is the PR ready to be reviewed ?

effjot commented 2 years ago

Nice, it seems you have added tests etc :) and it's green. Is the PR ready to be reviewed ?

I think so. ;-)

Previously, you had asked “Do you think you will need extra attributes related to raster later ? X/Y cell size, height/with of the raster, number of bands in the dataset...” For now, no extra attributes come to mind. Your examples could be queried from the raster. If the need arises, they could be added later and updated by the calculate_fields_from_data() trigger.

effjot commented 2 years ago

Oh, I just realised I should rebase this PR onto the current master.

effjot commented 2 years ago

@Gustry Can/should I rebase my PR branch on upstream/master, or would it mess up the PR here?

effjot commented 2 years ago

After @mdouchin pointed out the possible performance issues for large rasters, I’ve had a deeper look into ST_ConvexHull and ST_Envelope. They don’t differ much even for big rasters. But they both can be slow (0.5 to 2 seconds in my large example rasters).

However, I have realised a bigger problem due to the misunderstanding what a “raster” is: A table can contain many rasters in the PostGIS sense, which are tiles making up the complete raster in the QGIS sense. So both ST_ConvexHull and ST_Envelope don’t compute the hull of the whole thing, but of all the individual tiles, and the result is a table of geometries. These could by merged by ST_Union (either before or after ST_ConvexHull/ST_Envelope), which is very slow (in my examples up to 30 seconds).

Therefore I changed the code to use the extent from raster_columns, as @mdouchin suggested. This is fast.

I guess that almost all rasters will be rectangular and not rotated, skewed or otherwise irregular. Therefore the extent from raster_columns should be the same as the convex hull / envelope calculated from the target_table.

Another problem was the query for projection_authid, which I copied from the vector layer code. This was extremely slow (even slower than ST_ConvexHull+ST_Union; minutes for big rasters), so I took the SRID from raster_columns, too. If I understand correctly, it is not possible to for different rasters in a table to have different SRIDs, so it is not necessary to query target_table anyway.

effjot commented 2 years ago

Something cosmetic, but unrelated: I’ve found a wrong placeholder in the function update_postgresql_table_comment(). The string for the NOTICEs contains %s instead of %. I’ve fixed this. Should I include it in this PR?

mdouchin commented 2 years ago

Thanks a lot for having a deeper look at this performance issue. Indeed, a big raster can be a set of rows -> ok for me to use the already computed statistics in raster_columns

For the function update_postgresql_table_comment() feel free to change it here or in another PR, what is the easiest for you, as soon as the upgrade script is edited accordingly.

Regards

effjot commented 2 years ago

I have added the change in update_postgresql_table_comment() to this PR.

I hope everything’s alright now. :-)