Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ENH: Add WKT format support #979

Merged
merged 1 commit into from
May 1, 2022
Merged

ENH: Add WKT format support #979

merged 1 commit into from
May 1, 2022

Conversation

snowman2
Copy link
Contributor

Closes #977

@coveralls
Copy link

Coverage Status

Coverage increased (+0.04%) to 87.666% when pulling fafb779 on snowman2:wkt2 into 137853e on Toblerity:maint-1.9.

@mwtoews
Copy link
Member

mwtoews commented Apr 28, 2022

@snowman2 Fiona 1.9 has been updated to require GDAL >=3.1 (see b4dcc40), so a few simplifications could be made throughout this PR. I can review this when ready.

@mwtoews mwtoews self-assigned this Apr 28, 2022
@snowman2 snowman2 force-pushed the wkt2 branch 4 times, most recently from 7e41022 to 2e5bfec Compare April 29, 2022 01:08
@snowman2
Copy link
Contributor Author

@mwtoews, I think this is ready for a review.

fiona/_crs.pyx Outdated
_cpl.CPLFree(wkt_c)

if not wkt:
raise CRSError("Invalid input to create CRS: {}".format(crs))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This error should somehow also describe wkt_version, e.g. fiona._crs.crs_to_wkt(target_crs, wkt_version="abc") will raise an error that makes no mention of a potentially bad wkt_version value.

Not sure if it's possible to capture possible invalid wkt_version values earlier, or if that's a bad idea, as other values work, e.g. wkt_version="WKT2".

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am wondering if we could capture the underlying GDAL error message and raise it here. That may have more information about it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For now, I am adding the supported WKT versions to the error message on error.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there are a few ways to capture an error message, like this

Fiona/fiona/ogrext.pyx

Lines 1512 to 1516 in 9c8fe73

try:
exc_wrap_int(
OGR_L_SetAttributeFilter(cogr_layer, <const char*>where_c))
except CPLE_AppDefinedError as e:
raise AttributeFilterError(e) from None

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

Current error message:

fiona.errors.CRSError: Invalid input to create CRS: EPSG:4326. Please choose a different WKT version (WKT2 is recommended): WKT1 | WKT1_GDAL | WKT1_ESRI | WKT2 | WKT2_2018 | WKT2_2015. GDAL ERROR: Unsupported value for FORMAT

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Useful error capture, as sometimes WKT1_GDAL will be specified, but the error message would contain (e.g.) "GDAL ERROR: PROJ: proj_as_wkt: Unsupported conversion method: Lambert Conic Conformal (2SP Michigan)" as the chosen wkt format is incompatible.

fiona/__init__.py Outdated Show resolved Hide resolved
@snowman2 snowman2 force-pushed the wkt2 branch 3 times, most recently from 7ca5db2 to acb2d88 Compare April 29, 2022 19:16
Copy link
Member

@mwtoews mwtoews left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, looks great! Last request is to add an entry to CHANGES.txt (in the "New features" section), then I'll merge it after a few days (in case there is any further review by others).

@snowman2 snowman2 closed this May 1, 2022
@snowman2 snowman2 reopened this May 1, 2022
@mwtoews mwtoews merged commit e56b348 into Toblerity:maint-1.9 May 1, 2022
@mwtoews
Copy link
Member

mwtoews commented May 1, 2022

Thanks for the time an patience putting this together! The last CI failure is unrelated and is fixed with 7fa6bcc

@mwtoews mwtoews mentioned this pull request May 1, 2022
@snowman2 snowman2 deleted the wkt2 branch May 2, 2022 00:30
@snowman2
Copy link
Contributor Author

snowman2 commented May 2, 2022

Thanks @mwtoews 👍

List of a subset of field names to include on load.
wkt_version : str
WKT Version to use to for the CRS
(WKT1, WKT1_GDAL, WKT1_ESRI, WKT2, WKT2_2015, WKT2_2018).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for the Fiona API, I would suggest using WKT2_2019 which is the canonical name and remap that to WKT2_2018 when passing it to GDAL for compatibility with early GDAL 3.1 (WKT2_2019 was added in GDAL 3.2)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants