API: Raise warning when overriding a CRS via crs property setter

[JohnMoutafis]

Refs: #3085
For the current version: Warn the user that tries to manually change a GeoSeries' existing CRS by the property setter (ex. gs.crs = "new crs"), that the setter will be deprecated in future versions and that the set_crs method should be used instead.

[martinfleis]

As noted in #3085, we'll need to cover both GeoSeries and GeoDataFrame here.

[JohnMoutafis]

There seems to be an issue with GeoDataFrame.dissolve trying to set crs through the setter but I cannot wrap my head around were this is happening.
Any suggestions?

[martinfleis]

I've fixed the usage of GeoSeries.crs setter in GeoSeries.set_geometry and pushed one minor commit to ensure_geometry along the same lines.

A consequence of this warning is that due to usage of setter in some places and especially in tests, we have a ton of there warnings in our CI which we should ideally clear, especially if this will raise eventually.

[jorisvandenbossche]

@JohnMoutafis would you have time to update for the comments of @martinfleis ?

[JohnMoutafis]

@jorisvandenbossche I will try to find some (hopefully soon)

[martinfleis]

@JohnMoutafis given we'd like to have this included in 1.0 which shall be released hopefully soon, I have pushed some commits myself, mostly resolving my own comments.

[martinfleis]

Thanks for getting this going @JohnMoutafis!

[JohnMoutafis]

@martinfleis sorry for not finding the time to handle the requested changes.
Does the PR need anything atm to be merged?
What about the coverage?

[martinfleis]

I think it is ready to be merged, just want to wait for another review.

The coverage can be ignored I suppose, given the patch has 100%. It tends to be flaky from time to time.

[jorisvandenbossche]

Looks good, thanks!

[m-richards]

Sorry a bit late now, but these lines are missing spaces after the full stops inside the quotes (this also happens once above in the geodataframe warning).
I also see in the dev env that the warnings are getting triggered by pandas code in certain tests, which we may want to look into, but I suppose there's still quite a bit of time before the deprecation is enforced.

[jorisvandenbossche]

Sorry, missed that in re-reviewing, good catch! Will do a quick follow-up to fix

[jorisvandenbossche]

I also see in the dev env that the warnings are getting triggered by pandas code in certain tests, which we may want to look into, but I suppose there's still quite a bit of time before the deprecation is enforced.

I think they are being triggered in general in our tests. Looking at the ones in test_geodataframe.py, it seems this comes from doing df.crs = None (i.e. to remove the crs from a df for the test). But replacing that with df = df.set_crs(None, allow_override=True) doesn't work, because we don't accept None.

Do we want to accept None in that method, or do we not raise the warning for the case of assigning None ? (we are not silently replacing a crs, so less need to warn about that)

[martinfleis]

Yeah, I noted that in #3088 (review) and then forgot about it.

Do we want to accept None in that method, or do we not raise the warning for the case of assigning None ?

I think that we should have a canonical way of removing crs and set_crs(None, allow_override=True) feels like the best way.

In the tests, we can potentially replace that by assigning to the GeometryArray.crs but allowing None in set_crs might be the best.