-
Notifications
You must be signed in to change notification settings - Fork 224
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
Enable ruff's literal-membership (PLR6201) rule and fix violations #3317
Conversation
@@ -131,7 +131,7 @@ def _mock_ctypes_cdll_return(self, libname): | |||
# libname is a loaded GMT library | |||
return self.loaded_libgmt | |||
|
|||
@pytest.fixture() | |||
@pytest.fixture |
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.
Note that this change is to fix a PT001 rule violation that was triggered because of the preview mode setting. In non-preview mode, @pytest.fixture()
is valid, but in preview mode, @pytest.fixture()
is valid. See https://docs.astral.sh/ruff/rules/pytest-fixture-incorrect-parentheses-style/ and astral-sh/ruff#12106.
Three geopandas tests start to fail, but not sure if it's related to changes in this PR. |
pygmt/helpers/tempfile.py
Outdated
overflow = geojson[col].abs().max() > 2**31 - 1 | ||
schema["properties"][col] = "float" if overflow else "int32" | ||
ogrgmt_kwargs["schema"] = schema | ||
else: # GeoPandas v1.x. | ||
# The default engine "pyogrio" doesn't support the 'schema' parameter | ||
# but we can change the dtype directly. | ||
for col in geojson.columns: | ||
if geojson[col].dtype in ("int", "int64", "Int64"): | ||
if geojson[col].dtype in {"int", "int64", "Int64"}: |
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.
Three geopandas tests start to fail, but not sure if it's related to changes in this PR.
Interesting, I can reproduce the geopandas test failures locally, and it seems to be due to the change from ()
to {}
here on this line. Example test failure on pygmt.tests.test_geopandas.test_geopandas_plot_int64_as_float
:
baseline (correct) | result (incorrect) | diff |
---|---|---|
It seems to be because geojson[col].dtype
is a class instance like numpy.dtypes.Int64DType
rather than a str
, and that might be causing some issues with the membership test?
The fix is straightforward though:
if geojson[col].dtype in {"int", "int64", "Int64"}: | |
if geojson[col].dtype.name in {"int", "int64", "Int64"}: |
Description of proposed changes
Enable ruff's literal membership (PLR6201) rule to ensure a set literal is used when testing for membership. Note that this is a preview mode feature.
This offers a bit of a speed/performance improvement, as Python's peephole optimizer will optimize set membership tests (by converting the set to a frozenset), making it faster than tuple or list membership tests.
Example speed tests on my machine, about a 4.5x speedup using set
{}
compared to tuple()
or list[]
:References:
Addresses #2741 (comment)
Reminders
make format
andmake check
to make sure the code follows the style guide.doc/api/index.rst
.Slash Commands
You can write slash commands (
/command
) in the first line of a comment to performspecific operations. Supported slash command is:
/format
: automatically format and lint the code