Skip to content

[CLN] resolve circular Period dependency, prepare setup.py #21854

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

Merged
merged 4 commits into from
Jul 12, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 2 additions & 6 deletions pandas/_libs/__init__.py
Original file line number Diff line number Diff line change
@@ -1,9 +1,5 @@
# -*- coding: utf-8 -*-
# flake8: noqa

from .tslibs import iNaT, NaT, Timestamp, Timedelta, OutOfBoundsDatetime

# TODO
# period is directly dependent on tslib and imports python
# modules, so exposing Period as an alias is currently not possible
# from period import Period
from .tslibs import (
iNaT, NaT, Timestamp, Timedelta, OutOfBoundsDatetime, Period)
1 change: 1 addition & 0 deletions pandas/_libs/tslibs/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,5 +4,6 @@
from .conversion import normalize_date, localize_pydatetime, tz_convert_single
from .nattype import NaT, iNaT
from .np_datetime import OutOfBoundsDatetime
from .period import Period, IncompatibleFrequency
from .timestamps import Timestamp
from .timedeltas import delta_to_nanoseconds, ints_to_pytimedelta, Timedelta
10 changes: 8 additions & 2 deletions pandas/_libs/tslibs/resolution.pyx
Original file line number Diff line number Diff line change
Expand Up @@ -28,8 +28,6 @@ from timestamps import Timestamp

from pandas._libs.properties import cache_readonly

from pandas.core.algorithms import unique # TODO: Avoid this non-cython import

# ----------------------------------------------------------------------
# Constants

Expand Down Expand Up @@ -574,6 +572,10 @@ cdef class _FrequencyInferer(object):
if len(self.ydiffs) > 1:
return None

# lazy import to prevent circularity
# TODO: Avoid non-cython dependency
from pandas.core.algorithms import unique

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'm now leaning towards actually moving _FrequencyInferer back out of cython (it came from tseries.frequencies previously). The only place it is used is in frequencies.infer_freq, which itself is only used from DatetimelikeArrayMixin. i.e. it could reasonably be considered out-of-scope for tslibs, which is largely scalar-centric.

Moving it out would allow us to get rid of the unsightly khash dependency, which is the last non-src pandas dependency for tslibs.

Copy link
Contributor

Choose a reason for hiding this comment

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

sure that would be ok

Copy link
Member Author

Choose a reason for hiding this comment

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

great, will do this in follow up

if len(unique(self.fields['M'])) > 1:
return None

Expand Down Expand Up @@ -618,6 +620,10 @@ cdef class _FrequencyInferer(object):
# if not lib.ismember(wdiffs, set([4, 5, -47, -49, -48])).all():
# return None

# lazy import to prevent circularity
# TODO: Avoid non-cython dependency
from pandas.core.algorithms import unique

weekdays = unique(self.index.weekday)
if len(weekdays) > 1:
return None
Expand Down
2 changes: 2 additions & 0 deletions pandas/tests/tslibs/test_api.py
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,8 @@ def test_namespace():
api = ['NaT',
'iNaT',
'OutOfBoundsDatetime',
'Period',
'IncompatibleFrequency',
'Timedelta',
'Timestamp',
'delta_to_nanoseconds',
Expand Down
89 changes: 39 additions & 50 deletions setup.py
Original file line number Diff line number Diff line change
Expand Up @@ -24,23 +24,6 @@ def is_platform_windows():
return sys.platform == 'win32' or sys.platform == 'cygwin'


def is_platform_linux():
return sys.platform == 'linux2'


def is_platform_mac():
return sys.platform == 'darwin'


min_cython_ver = '0.28.2'
try:
import Cython
ver = Cython.__version__
_CYTHON_INSTALLED = ver >= LooseVersion(min_cython_ver)
except ImportError:
_CYTHON_INSTALLED = False


min_numpy_ver = '1.9.0'
setuptools_kwargs = {
'install_requires': [
Expand All @@ -53,24 +36,29 @@ def is_platform_mac():
}
Copy link
Member Author

Choose a reason for hiding this comment

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

If there are no objections, I'd like to move this dictionary up above the min_cython_version = '0.28.2 line

Copy link
Contributor

Choose a reason for hiding this comment

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

sure



min_cython_ver = '0.28.2'
try:
import Cython
ver = Cython.__version__
_CYTHON_INSTALLED = ver >= LooseVersion(min_cython_ver)
except ImportError:
_CYTHON_INSTALLED = False

# The import of Extension must be after the import of Cython, otherwise
# we do not get the appropriately patched class.
# See https://cython.readthedocs.io/en/latest/src/reference/compilation.html
from distutils.extension import Extension # noqa:E402
from distutils.command.build import build # noqa:E402
from distutils.command.build_ext import build_ext as _build_ext # noqa:E402

try:
if not _CYTHON_INSTALLED:
raise ImportError('No supported version of Cython installed.')
try:
from Cython.Distutils.old_build_ext import old_build_ext as _build_ext # noqa:F811,E501
except ImportError:
# Pre 0.25
from Cython.Distutils import build_ext as _build_ext
from Cython.Distutils.old_build_ext import old_build_ext as _build_ext
cython = True
except ImportError:
from distutils.command.build_ext import build_ext as _build_ext
cython = False


if cython:
else:
try:
try:
from Cython import Tempita as tempita
Expand Down Expand Up @@ -103,27 +91,30 @@ def is_platform_mac():


class build_ext(_build_ext):
def build_extensions(self):
@classmethod
def render_templates(cls, pxifiles):
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 needed to be able to call this method separately to get cythonize working.

for pxifile in pxifiles:
# build pxifiles first, template extension must be .pxi.in
assert pxifile.endswith('.pxi.in')
outfile = pxifile[:-3]

# if builing from c files, don't need to
# generate template output
if cython:
for pxifile in _pxifiles:
# build pxifiles first, template extension must be .pxi.in
assert pxifile.endswith('.pxi.in')
outfile = pxifile[:-3]

if (os.path.exists(outfile) and
os.stat(pxifile).st_mtime < os.stat(outfile).st_mtime):
# if .pxi.in is not updated, no need to output .pxi
continue
if (os.path.exists(outfile) and
os.stat(pxifile).st_mtime < os.stat(outfile).st_mtime):
# if .pxi.in is not updated, no need to output .pxi
continue

with open(pxifile, "r") as f:
tmpl = f.read()
pyxcontent = tempita.sub(tmpl)
with open(pxifile, "r") as f:
tmpl = f.read()
pyxcontent = tempita.sub(tmpl)

with open(outfile, "w") as f:
f.write(pyxcontent)
with open(outfile, "w") as f:
f.write(pyxcontent)

def build_extensions(self):
# if building from c files, don't need to
# generate template output
if cython:
self.render_templates(_pxifiles)

numpy_incl = pkg_resources.resource_filename('numpy', 'core/include')

Expand Down Expand Up @@ -360,7 +351,6 @@ def run(self):
class CheckingBuildExt(build_ext):
"""
Subclass build_ext to get clearer report if Cython is necessary.

"""

def check_cython_extensions(self, extensions):
Expand All @@ -379,9 +369,11 @@ def build_extensions(self):


class CythonCommand(build_ext):
"""Custom distutils command subclassed from Cython.Distutils.build_ext
"""
Custom distutils command subclassed from Cython.Distutils.build_ext
to compile pyx->c, and stop there. All this does is override the
C-compile method build_extension() with a no-op."""
C-compile method build_extension() with a no-op.
"""
def build_extension(self, ext):
pass

Expand Down Expand Up @@ -445,7 +437,6 @@ def srcpath(name=None, suffix='.pyx', subdir='src'):
lib_depends.append('pandas/_libs/src/util.pxd')
else:
lib_depends = []
plib_depends = []

common_include = ['pandas/_libs/src/klib', 'pandas/_libs/src']

Expand All @@ -471,8 +462,6 @@ def pxd(name):

tseries_depends = np_datetime_headers + ['pandas/_libs/tslibs/np_datetime.pxd']

# some linux distros require it
Copy link
Contributor

Choose a reason for hiding this comment

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

hmm do we need 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.

Apparently not. It has apparently sat there unused for a while.

libraries = ['m'] if not is_platform_windows() else []

ext_data = {
'_libs.algos': {
Expand Down