search

north-road / qgis-processing-r

QGIS Processing R Provider Plugin
https://north-road.github.io/qgis-processing-r/
GNU General Public License v3.0
63 stars 14 forks source link

Minor bug fixes #99

Closed gavg712 closed 3 years ago

gavg712 commented 3 years ago

Three minor changes are proposed.

  1. When using a multiple vector layer input the concatenation with c() breaks the structure of the spatial objects.The proposal is to replace c() with list() so that the structures of the vector spatial objects are maintained. There would be no problem with rasters either, because currently an object of class "list" is already returned.

  2. The crs input currently returns a string with the AuthID of the crs. It works fine with any CRS registered by an Authority ID. But it does not work with a CRS customized by the user. The proposal is to replace the AuthID text with WKT. That way it can be used with both sp and sf, raster and rgdal (#98 ).

  3. The CSV Output function write a table with row names included. This row names are shown has a "field1" column in QGIS. The porposal is to avoid the row names export by include the argument row.names = FALSE in write.csv()

nyalldawson commented 3 years ago

The crs input currently returns a string with the AuthID of the crs. It works fine with any CRS registered by an Authority ID. But it does not work with a CRS customized by the user.

Good catch!

Can I suggest a small refinement to this logic, and to first check if the authid starts with 'epsg:' before resorting to the wkt representation. Wkt is very verbose and hard to quickly read, so if we can avoid it in some circumstances we should.

Additionally we should pass the format argument to QGIS' wkt method and request the wkt in the wkt2 format, as the default will give the lossy wkt1 representation.

nyalldawson commented 3 years ago

When using a multiple vector layer input the concatenation with c() breaks the structure of the spatial objects.The proposal is to replace c() with list()

@JanCaha does this change sound good to you?

gavg712 commented 3 years ago

Can I suggest a small refinement to this logic, and to first check if the authid starts with 'epsg:' before resorting to the wkt representation. Wkt is very verbose and hard to quickly read, so if we can avoid it in some circumstances we should.

Since the problem jumps with the user CRS, then wouldn't it be better to check if it starts with "USER:". If it starts with "USER:", then it would return the WKT2 otherwise it would return the AuthID. Do you agree?

Additionally we should pass the format argument to QGIS' wkt method and request the wkt in the wkt2 format, as the default will give the lossy wkt1 representation.

Do you mean to use crs.toWkt(variant=4)? Which variant should be used? (from here )

nyalldawson commented 3 years ago

. If it starts with "USER:", then it would return the WKT2 otherwise it would return the AuthID. Do you agree?

That sounds good! Also better check for empty strings too, as that can also happen.

crs.toWkt(variant=4)? Which variant should be used? (from here )

Use

crs.toWkt(QgsCoordinateReferenceSystem.WKT2_2019_SIMPLIFIED)

(Simplified removes line breaks and leading indentation)

nyalldawson commented 3 years ago

LGTM! I'll just wait for @JanCaha's feedback regarding the list change and then we can merge

JanCaha commented 3 years ago

This all seems to be ok. I did some tests in R and I did not meet any problems with this new code. These are some nice improvements.

This might be worthy of relasing new version ;-)

nyalldawson commented 3 years ago

I'll push a new version today!

nyalldawson commented 3 years ago

Done - 3.1.0 has just been pushed.