search

inbo / n2khab

R package with preprocessing functions and standard reference data for Flemish Natura 2000 (N2K) habitat (HAB) analyses
https://inbo.github.io/n2khab
GNU General Public License v3.0
2 stars 1 forks source link

Add explicit support for terra and fix CRS for terra - unless terra changes approach #132

Closed florisvdh closed 3 years ago

florisvdh commented 3 years ago

Since for some files the read_xxx() functions explicitly set the CRS in order to replace ESRI:102499 (in the file) by EPSG:31370 (https://github.com/inbo/n2khab/issues/85#issuecomment-741604642), or in order to stay tuned with the EPSG WKT definitions as delivered by GDAL or PROJ, the same may need to be considered when working with terra.

terra appears to re-read the (potentially non-EPSG conform) CRS from the file instead of inheriting the WKT string from the raster object, when converting from the raster object. The latter may be regarded a bug in terra, maybe not. Anyhow, it will cause CRS clashes with other (effectively EPSG:31370) objects, as shown in this screenshot from @hansvancalster (visible from the printed proj4string - although it's actually the WKT strings that matter!):

afbeelding

Partially depending on the outcome when discussing with terra, we may adapt on our side by providing an extra argument for read_GRTSmh() and its sisters to allow returning a terra object right away, with CRS set appropriately.

@hansvancalster further ideas on the latter? raster output could stay the default, while terra output becomes an option. But we could consider the reverse, if you think it's time to do that.

Note: when well controlling the CRS, i.e. not through proj4strings but WKT, one can suppress the rgdal warnings about proj4string degradation using options(rgdal_show_exportToProj4_warnings = "none").

hansvancalster commented 3 years ago

@hansvancalster further ideas on the latter? raster output could stay the default, while terra output becomes an option.

Good idea

But we could consider the reverse, if you think it's time to do that.

Too early in my opinion. Documentation is behind compared to raster and many more packages depend on raster than on terra (this is ofcourse not surprising for a new package).

florisvdh commented 3 years ago

@hansvancalster Since this has been solved in https://github.com/rspatial/terra/issues/200, I'd rather drop or postpone the idea of returning terra objects as an option - this was primarily a need in order to fix the CRS bug. What do you think?

Having to support multiple representations inside our functions means more maintenance, and there's also stars and sp with still another raster representation. Rather, conversions should always be possible by the user, based on the output from n2khab. But we may consider moving to terra or stars as a default, at some point in the future.

hansvancalster commented 3 years ago

I agree, better to keep maintenance low and rely on conversions provided by the other packages. For the moment you need the development version of terra, but once 1.2-8 is on cran the conversion will work as expected.