-
Notifications
You must be signed in to change notification settings - Fork 902
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
MAINT: Reduce warnings in CI #2966
MAINT: Reduce warnings in CI #2966
Conversation
@m-richards what is the state of this? With pandas 2.1, some of the warnings are propagated to a user (categorical dtype checks). Would be good to resolve that in 0.14. |
I'll have another look, I think it's left as draft as there was more that could be done, but there's probably enough changes in this PR as is already. Edit: I think this is fine to review as is, although I can add to it if there are other warnings that are missing and particularly noisy. |
# Conflicts: # geopandas/tests/test_pandas_methods.py
geopandas/geoseries.py
Outdated
def apply(self, func, convert_dtype: bool = True, args=(), **kwargs): | ||
result = super().apply(func, convert_dtype=convert_dtype, args=args, **kwargs) | ||
def apply(self, func, convert_dtype: bool = None, args=(), **kwargs): | ||
if convert_dtype is not None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just for clarity since this is a bit ugly.
In pandas 2.1, convert dtype is deprecated and the future behaviour is the same as convert_dtype = True (pandas-dev/pandas#52786)
So this just preserves the existing signature and forwards what the user provides. If we have the default of None, we pass nothing through on pandas 2.1 to get the convert_dtype=True behaviour without the warning
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I pushed the same change of categorical dtype from plot to explore but the rest looks great! Thanks for this!
geopandas/tests/test_geoseries.py
Outdated
"ignore", "The indices of the two GeoSeries are different", UserWarning | ||
) | ||
assert_array_equal( | ||
self.a1.geom_almost_equals(self.a2, align=True), [False, True, False] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we actually be warning here? (this warning is coming from ourselves, right?) Since the keyword is explicitly set
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's what you'd expect right? That's not how its implemented though (xref #1832), there is no way to silence the warning currently if the index is unaligned.
geopandas/tests/test_overlay.py
Outdated
result1 = overlay(df1, df2, keep_geom_type=True) | ||
with warnings.catch_warnings(): | ||
warnings.filterwarnings( | ||
"ignore", "`unary_union` returned None due to all-None", FutureWarning | ||
) | ||
result1 = overlay(df1, df2, keep_geom_type=True) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is actually something we should ideally fix in overlay
itself, as the user shouldn't see this warning (it will not change any behaviour, because the resulting geometry collection will also get dropped afterwards).
So I would suggest to not hide this warning here in our tests (and just leave it as a warning for now for this PR)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree that makes sense. I'll add the warning back in, and I might move this to another issue since it's actually bubbled up from a dissolve call within overlay, so needs some examining to work out where to fix. (Or if we just roll over the deprecation and change the behaviour that won't be necessary).
…o reduce_warnings
# Conflicts: # geopandas/tests/test_geoseries.py
# Conflicts: # geopandas/tests/test_geodataframe.py
In the last commit (09189ce), I reverted a change to silence a warning in a groupby test. Because with this change, the test is now failing with latest geopandas (maybe related to the _constructor changes?). In any case something to further investigate, but let's do that in a follow-up such that this PR can be merged (I also want to do a general follow-up for fixing more warnings). |
Related to that last commit, a reproducer from that test: from shapely.geometry import Point
from geopandas import GeoDataFrame
crs = "EPSG:4326"
df = GeoDataFrame(
{
"geometry": [Point(0, 0), Point(1, 1), Point(0, 0)],
"value1": np.arange(3, dtype="int64"),
"value2": np.array([1, 2, 1], dtype="int64"),
},
crs=crs,
)
# dummy test asserting we can access the crs
def func(group):
assert isinstance(group, GeoDataFrame)
assert group.crs == crs
# original code, warning with latest pandas about group key being included in applied group
df.groupby("value2").apply(func)
# proposed fix to avoid the warning, now failing
df.groupby("value2")[df.columns].apply(func) This is failing since #3080, but I think this is rather a bug on the pandas side (not calling finalize when doing the column selection, and thus the active geometry name is not passed on), and essentially similar as the other follow-up issue from #3080: this was previously only working because of passing the data to GeoDataFrame which then defaults to "geometry" column. If changing the above example to use a different column name, it was already failing before #3080 |
Co-authored-by: Martin Fleischmann <martin@martinfleischmann.net> Co-authored-by: Joris Van den Bossche <jorisvandenbossche@gmail.com>
Co-authored-by: Martin Fleischmann <martin@martinfleischmann.net> Co-authored-by: Joris Van den Bossche <jorisvandenbossche@gmail.com>
No description provided.