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.

Sign up for GitHub

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

Jump to bottom

CLN: trying isort-dev #32489

Closed
wants to merge 2 commits into from
Closed

CLN: trying isort-dev #32489

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
30dabbc
CLN: trying isort-dev
Mar 6, 2020
473833b
Removing useless "as"
Mar 6, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
45 changes: 22 additions & 23 deletions pandas/_libs/lib.pyx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,29 +2,29 @@ from collections import abc
from decimal import Decimal
from fractions import Fraction
from numbers import Number

import sys

import cython
from cython import Py_ssize_t

from cpython.object cimport PyObject_RichCompareBool, Py_EQ
from cpython.ref cimport Py_INCREF
from cpython.tuple cimport PyTuple_SET_ITEM, PyTuple_New
from cpython.iterator cimport PyIter_Check
from cpython.sequence cimport PySequence_Check
from cpython.number cimport PyNumber_Check

from cpython.datetime cimport (
PyDateTime_Check,
PyDate_Check,
PyTime_Check,
PyDelta_Check,
PyDateTime_Check,
PyDateTime_IMPORT,
PyDelta_Check,
PyTime_Check,
)
from cpython.iterator cimport PyIter_Check
from cpython.number cimport PyNumber_Check
from cpython.object cimport Py_EQ, PyObject_RichCompareBool
from cpython.ref cimport Py_INCREF
from cpython.sequence cimport PySequence_Check
from cpython.tuple cimport PyTuple_New, PyTuple_SET_ITEM

PyDateTime_IMPORT

import numpy as np

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if there is a viable way to avoid this newline (and the one on 45), id like to maintain the pattern of keeping all the numpy imports together

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I couldn't achieve that unfortunately, my guess is that isort is separates import and cimport even if it's importing the same library/module

cimport numpy as cnp
from numpy cimport (
NPY_OBJECT,
Expand All @@ -42,6 +42,7 @@ from numpy cimport (
uint8_t,
uint64_t,
)

cnp.import_array()

cdef extern from "numpy/arrayobject.h":
Expand All @@ -65,24 +66,23 @@ cdef extern from "numpy/arrayobject.h":
cdef extern from "src/parse_helper.h":
int floatify(object, float64_t *result, int *maybe_int) except -1

cimport pandas._libs.util as util
from pandas._libs.util cimport is_nan, UINT64_MAX, INT64_MAX, INT64_MIN
from pandas._libs cimport util
from pandas._libs.util cimport INT64_MAX, INT64_MIN, UINT64_MAX, is_nan

from pandas._libs.tslib import array_to_datetime
from pandas._libs.tslibs.nattype cimport NPY_NAT, c_NaT as NaT
from pandas._libs.tslibs.conversion cimport convert_to_tsobject
from pandas._libs.tslibs.timedeltas cimport convert_to_timedelta64
from pandas._libs.tslibs.timezones cimport get_timezone, tz_compare

from pandas._libs.missing cimport (
C_NA,
checknull,
isnaobj,
is_null_datetime64,
is_null_timedelta64,
is_null_period,
C_NA,
is_null_timedelta64,
isnaobj,
)

from pandas._libs.tslibs.conversion cimport convert_to_tsobject
from pandas._libs.tslibs.nattype cimport NPY_NAT, c_NaT as NaT
from pandas._libs.tslibs.timedeltas cimport convert_to_timedelta64
from pandas._libs.tslibs.timezones cimport get_timezone, tz_compare
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

putting tslibs imports before non-tslibs imports (within cython files) is intentional, can we update config to preserve this?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is an option to have groups in isort and then order them, so if we have two modules foo and bar, and we want bar first, we can do:

(in setup.cfg)

[isort]
known_foo = pandas.foo
known_bar = pandas.bar
sections = bar,foo

we can achieve the desired behavior but it will be applied across all files (py, pyx, pxi).

Edit:

I have opened an issue at isort asking that question.

ref: PyCQA/isort#1162


# constants that will be compared to potentially arbitrarily large
# python int
Expand Down Expand Up @@ -1320,8 +1320,7 @@ def infer_dtype(value: object, skipna: bool = True) -> str:
else:
if not isinstance(value, list):
value = list(value)
from pandas.core.dtypes.cast import (
construct_1d_object_array_from_listlike)
from pandas.core.dtypes.cast import construct_1d_object_array_from_listlike
values = construct_1d_object_array_from_listlike(value)

# make contiguous
Expand Down
15 changes: 8 additions & 7 deletions pandas/core/frame.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@
alignment and a host of useful data manipulation methods having to do with the
labeling information
"""

import collections
from collections import abc
import datetime
Expand All @@ -35,7 +34,7 @@
import warnings

import numpy as np
import numpy.ma as ma
from numpy import ma

from pandas._config import get_option

Expand Down Expand Up @@ -133,6 +132,7 @@

if TYPE_CHECKING:
from pandas.core.groupby.generic import DataFrameGroupBy

from pandas.io.formats.style import Styler

# ---------------------------------------------------------------------
Expand Down Expand Up @@ -438,7 +438,7 @@ def __init__(
elif isinstance(data, dict):
mgr = init_dict(data, index, columns, dtype=dtype)
elif isinstance(data, ma.MaskedArray):
import numpy.ma.mrecords as mrecords
from numpy.ma import mrecords
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm actually now that I think about it is this an isort thing or something you are suggesting as a standard?

These aren't entirely equivalent and this change is more prone to circular imports. The former is actually suggest by Guido as the way to go so I don't think this is worth changing. (can't find link atm, but its somewhere in the Python docs)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is in fact an isort thing (in the dev version), If you can find the link I think we should open an issue about this (to isort).

The first commit is the changes made by isort, the second commit was done manually to fix the pattern of:

from foo.bar import baz as baz

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

import numpy.ma doesnt import mrecords into the np.ma namespace. Is this pattern ever liable to trip us up with either form of import?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@MomIsBestFriend here is what I was describing:

https://docs.python.org/3/faq/programming.html#how-can-i-have-modules-that-mutually-import-each-other

Guido van Rossum recommends avoiding all uses of from import ..., and placing all code inside functions.

So while this is inside a function, I think still not worth changing to a usage that is discouraged by Guido himself

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @WillAyd for sending the link!

I have opened an issue about this discussion at isort.

xref PyCQA/isort#1164

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Regardless of the from x import y conversation, these usages are not needed since you are not renaming:

import numpy.na.mrecords is the same as import numpy.na.mrecords as mrecords (will probably help you at least workaround this decision by isort team).


# masked recarray
if isinstance(data, mrecords.MaskedRecords):
Expand Down Expand Up @@ -4621,8 +4621,9 @@ def duplicated(
-------
Series
"""
from pandas._libs.hashtable import _SIZE_HINT_LIMIT, duplicated_int64

from pandas.core.sorting import get_group_index
from pandas._libs.hashtable import duplicated_int64, _SIZE_HINT_LIMIT

if self.empty:
return Series(dtype=bool)
Expand Down Expand Up @@ -5455,7 +5456,7 @@ def combine_first(self, other: "DataFrame") -> "DataFrame":
1 0.0 3.0 1.0
2 NaN 3.0 1.0
"""
import pandas.core.computation.expressions as expressions
from pandas.core.computation import expressions

def extract_values(arr):
# Does two things:
Expand Down Expand Up @@ -5602,7 +5603,7 @@ def update(
1 2 500.0
2 3 6.0
"""
import pandas.core.computation.expressions as expressions
from pandas.core.computation import expressions

# TODO: Support other joins
if join != "left": # pragma: no cover
Expand Down Expand Up @@ -7161,8 +7162,8 @@ def join(
def _join_compat(
self, other, on=None, how="left", lsuffix="", rsuffix="", sort=False
):
from pandas.core.reshape.merge import merge
from pandas.core.reshape.concat import concat
from pandas.core.reshape.merge import merge

if isinstance(other, Series):
if other.name is None:
Expand Down