-
Notifications
You must be signed in to change notification settings - Fork 901
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
Raise when overriding a CRS via crs property setter #3085
Milestone
Comments
I just had a quick look at #3088, @martinfleis I think you mean that we should do this for both GeoSeries.crs and GeoDataFrame.crs, correct? (that PR is currently just GeoSeries) |
Yes, we should surely do it for both. |
The merged PR in the end also did it for GeoDataFrame, so we can close this issue (I started doing a PR for finishing this, but so then only updating the changelog -> #3333) |
Closed by #3088 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
When using
set_crs
, we raise a ValueError when the GeoSeries already has a CRS which is not equal to the passed CRS. But when assigning directly to .crs asgdf.crs = 4326
, we just let user to override the existing CRS, which is a quite common error.I propose to add checks to crs property setter and allow assignment only when current crs is None (or equal but None feels safer) and point users either to
set_crs(crs, allow_override=True)
if they really want to override it or toto_crs(crs)
, which in most cases will be what that actually wanted to do.This will probably require a deprecation period where we first warn and then raise in a subsequent release. It would be nice to make that switch to raise in 1.0 but I don't think there's 0.15 coming in the meantime, so let's start with a warning.
The text was updated successfully, but these errors were encountered: