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

Avoid Impact storing a pyproj.CRS object #713

Merged
merged 8 commits into from
May 16, 2023
Merged

Conversation

peanutfun
Copy link
Member

@peanutfun peanutfun commented May 4, 2023

Changes proposed in this PR:

  • Transform pyproj.CRS into WKT string in Impact.__init__. WKT is the most precise form of storing CRS information, apart from an actual CRS object. This should work fine as the Impact.crs attribute can now safely be stored as string, and inserting the WKT into the Exposures constructor is supported.
  • Fix a subtle bug in an impact test

This PR fixes #706

PR Author Checklist

PR Reviewer Checklist

@peanutfun
Copy link
Member Author

@ChrisFairless could you check if this resolves your issue? Instead of storing CRS objects, I resorted to converting them into WKT strings when passing them to the Impact constructor

@ChrisFairless
Copy link
Collaborator

Yes this looks good! Thanks for the fix.

I like how the WKT is recognised by rasterio.crs.CRS.from_user_input so we can still use this to check CRS equality.

@emanuel-schmid
Copy link
Collaborator

emanuel-schmid commented May 12, 2023

Agreed. In my experience, WKT is the one that gives the least amount of warnings. 😁
I've added the crs to the attributes list so it's clear that it is a string.

  • but then ... maybe it's not. What if it's a rasterio.crs.CRS?

@peanutfun
Copy link
Member Author

rasterio.crs.CRS supports to_wkt, too: https://rasterio.readthedocs.io/en/latest/api/rasterio.crs.html#rasterio.crs.CRS.to_wkt

I'll patch that.

@peanutfun
Copy link
Member Author

@emanuel-schmid Fixed handling of rasterio.crs.CRS here: a5ab8bd

@emanuel-schmid
Copy link
Collaborator

Many thanks! To me it's fine for merging, I've added a changlog entry. Feel free to edit.

@peanutfun
Copy link
Member Author

@emanuel-schmid
Copy link
Collaborator

emanuel-schmid commented May 15, 2023

🤷 I's just suppress it for this particular line by adding # pylint: disable=no-name-in-module at the end.
There are only 2 of this kind throughout climada_python so changing .pylintrc seems over-reacting.

@peanutfun peanutfun merged commit 57086eb into develop May 16, 2023
@emanuel-schmid emanuel-schmid deleted the impact-write-crs branch May 25, 2023 08:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants