CLN: Cython Py2/3 Compatible Imports #23940

WillAyd · 2018-11-27T03:31:31Z

progress towards Cython language_level Warning - switch to Python3? #23927
tests added / passed
passes git diff upstream/master -u -- "*.py" | flake8 --diff
whatsnew entry

@jbrockmendel

pep8speaks · 2018-11-27T03:31:34Z

Hello @WillAyd! Thanks for submitting the PR.

There are no PEP8 issues in the file setup.py !

WillAyd · 2018-11-27T03:38:26Z

Main things needed to make this work were:

Changing import machinery (95%)
Handling difference in division (5%)
Removed unicode from fused type with string / bytes object

Not entirely sure of the impact of the last. There is an alternate language setting in Cython we could use which uses Python3 but maintains 2 unicode semantics if this doesn't end up passing

jbrockmendel · 2018-11-27T03:49:15Z

pandas/_libs/groupby.pyx

@@ -54,10 +56,10 @@ cdef inline float64_t median_linear(float64_t* a, int n) nogil:
        n -= na_count

    if n % 2:
-        result = kth_smallest_c( a, n / 2, n)
+        result = kth_smallest_c( a, n // 2, n)


Space after open paren

I'm not sure if it changes with language-level, but in cython / defaults to cdivision, which behaves like python floor-division (... mostly. its corner cases are bizarre). The cython docs says there is a non-trivial perf impact of disabling cdivision. It might be worth checking how it treats floordiv.

Then again, this isn't an op we're doing zillions of times, so not a huge deal.

If you try to just use / here compilation will fail as you try to assign a double to an int

jbrockmendel · 2018-11-27T14:48:29Z

How strongly do you feel about absolute vs relative imports? These get pretty verbose.

WillAyd · 2018-11-27T19:38:34Z

Definitely more verbose but since we tend to favor absolute in the rest of the code base I think the consistency here is nice.

jorisvandenbossche · 2018-11-27T21:31:30Z

I would be in favor of doing explicit relative imports here for imports within _libs

WillAyd · 2018-11-28T02:01:26Z

Just so I'm clear do you only want those imports to be relative from modules within the tslibs folder or from any module? For instance if you look at index.pyx it currently has:

from pandas._libs.tslibs.conversion cimport maybe_datetimelike_to_i8

So do we want this to be from .tslibs.conversion import maybe_datetimelike_to_i8 or should this stay absolute since it's not in the same directory?

Note that there is a .util module in the higher level directory and in tslibs. Full name spacing makes it a little bit clearer which one is being imported so I still slightly prefer that, but if I'm the odd man out on this one certainly glad to change

WillAyd · 2018-11-28T02:02:34Z

Side note - I repurposed this PR to just focus on imports for the time being. I think the actual changes to support Python3 syntax make more sense to do in a follow up. I think the unicode fused type is going to be finicky so may even wait for that until we officially drop Py2 support soon

codecov · 2018-12-01T21:54:25Z

Codecov Report

Merging #23940 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master   #23940   +/-   ##
=======================================
  Coverage   42.46%   42.46%           
=======================================
  Files         161      161           
  Lines       51557    51557           
=======================================
  Hits        21892    21892           
  Misses      29665    29665

Flag	Coverage Δ
#single	`42.46% <ø> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5b0610b...4d0437f. Read the comment docs.

codecov · 2018-12-01T21:54:26Z

Codecov Report

Merging #23940 into master will increase coverage by <.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master   #23940      +/-   ##
==========================================
+ Coverage    92.3%   92.31%   +<.01%     
==========================================
  Files         163      163              
  Lines       51977    51977              
==========================================
+ Hits        47979    47980       +1     
+ Misses       3998     3997       -1

Flag	Coverage Δ
#multiple	`90.71% <ø> (ø)`	⬆️
#single	`42.99% <ø> (-0.01%)`	⬇️

Impacted Files	Coverage Δ
pandas/util/testing.py	`87.84% <0%> (+0.09%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8b4b38f...b908e69. Read the comment docs.

jbrockmendel · 2018-12-02T01:08:22Z

On further thought, I think Joris's suggestion is a really good one. A lot of effort has gone into making _libs modules, particularly _libs.tslibs self-contained. Using relative imports make it really obvious that a module has no one-level-up dependencies.

pandas/_libs/tslibs/nattype.pyx

jreback

let's just use absolute imports

On further thought, I think Joris's suggestion is a really good one. A lot of effort has gone into making _libs modules, particularly _libs.tslibs self-contained. Using relative imports make it really obvious that a module has no one-level-up dependencies.

this then is a departure from all other imports and so am -1 on this.

jreback · 2018-12-03T15:02:39Z

pandas/_libs/tslibs/ccalendar.pyx

@@ -10,7 +10,7 @@ from cython import Py_ssize_t
 from numpy cimport int64_t, int32_t

 from locale import LC_TIME
-from strptime import LocaleTime
+from .strptime import LocaleTime


why not change this one?

jreback · 2018-12-03T15:03:23Z

pandas/_libs/tslibs/conversion.pyx

-
-from timedeltas cimport cast_from_unit
-from timezones cimport (is_utc, is_tzlocal, is_fixed_offset,
+from .ccalendar import DAY_SECONDS, HOUR_SECONDS


why are some of these relative? I think this is very confusing, I would rather have all absolute imports as we do elsewhere.

WillAyd · 2018-12-06T20:11:58Z

OK back to absolute @jreback PTAL

jreback · 2018-12-23T23:28:51Z

got lost in the mix, can you merge master.

WillAyd · 2018-12-25T04:06:15Z

Hypothesis test failure should be unrelated @jreback PTAL

jreback · 2018-12-26T00:23:04Z

can you merge master. lgtm. ping on green.

WillAyd · 2018-12-26T23:50:15Z

@jreback green

jreback · 2018-12-26T23:50:53Z

thanks @WillAyd

* upstream/master: (26 commits) DOC: Fixing doc upload (no such remote origin) (pandas-dev#24459) BLD: for C extension builds on mac, target macOS 10.9 where possible (pandas-dev#24274) POC: _eadata (pandas-dev#24394) DOC: Correct location (pandas-dev#24456) CI: Moving CircleCI build to Travis (pandas-dev#24449) BUG: Infer compression by default in read_fwf() (pandas-dev#22200) DOC: Fix minor typo in whatsnew (pandas-dev#24453) DOC: Add dateutil to intersphinx pandas-dev#24437 (pandas-dev#24443) DOC: Adding links to offset classes in timeseries.rst (pandas-dev#24448) DOC: Adding offsets to API ref (pandas-dev#24446) DOC: fix flake8 issue in groupby.rst (pandas-dev#24363) DOC: Fixing more doc warnings (pandas-dev#24438) API: Simplify repeat signature (pandas-dev#24447) BUG: to_datetime(Timestamp, utc=True) localizes to UTC (pandas-dev#24441) CLN: Cython Py2/3 Compatible Imports (pandas-dev#23940) DOC: Fixing more doc warnings (pandas-dev#24431) DOC: Removing old release.rst (pandas-dev#24427) BUG-24408 Series.dt does not maintain own copy of index (pandas-dev#24426) DOC: Fixing several doc warnings (pandas-dev#24430) ENH: fill_value argument for shift pandas-dev#15486 (pandas-dev#24128) ...

WillAyd added 3 commits November 26, 2018 22:08

Updated setup

0c84dcb

Conversion to absolute imports

0e90b55

Missed imports, division, unicode compat

62bee57

jbrockmendel reviewed Nov 27, 2018

View reviewed changes

WillAyd added 2 commits November 27, 2018 01:30

Missing import resolution for strptime

43c8098

Timestamp import failure

385e2da

TomAugspurger mentioned this pull request Nov 27, 2018

ImportError (import pandas) #23798

Closed

WillAyd added 3 commits November 27, 2018 17:51

Merge remote-tracking branch 'upstream/master' into cython-py3-compat

836f3f2

Reverted non-import changes

7522818

Reverted floor div changes

ea818aa

WillAyd changed the title ~~Cython py3 lanuage_level~~ CLN: Cython Py2/3 Compatible Imports Nov 28, 2018

WillAyd added the Clean label Nov 28, 2018

WillAyd added 2 commits December 1, 2018 12:58

LINT fixup

1d6d196

Merge remote-tracking branch 'upstream/master' into cython-py3-compat

4d0437f

WillAyd added 2 commits December 2, 2018 22:04

Merge remote-tracking branch 'upstream/master' into cython-py3-compat

c78d1a9

Relative imports in tslibs folder

5d09285

WillAyd commented Dec 3, 2018

View reviewed changes

pandas/_libs/tslibs/nattype.pyx Show resolved Hide resolved

Missed relative imports

db2acb4

jreback requested changes Dec 3, 2018

View reviewed changes

Back to absolute

f25c0d0

WillAyd added 2 commits December 3, 2018 23:19

Merge remote-tracking branch 'upstream/master' into cython-py3-compat

9034cba

Merge remote-tracking branch 'upstream/master' into cython-py3-compat

c3bce2f

Merge remote-tracking branch 'upstream/master' into cython-py3-compat

877b3d5

jreback added this to the 0.24.0 milestone Dec 26, 2018

jreback approved these changes Dec 26, 2018

View reviewed changes

Merge remote-tracking branch 'upstream/master' into cython-py3-compat

b908e69

jreback merged commit 1d86861 into pandas-dev:master Dec 26, 2018

WillAyd deleted the cython-py3-compat branch December 26, 2018 23:51

Pingviinituutti pushed a commit to Pingviinituutti/pandas that referenced this pull request Feb 28, 2019

CLN: Cython Py2/3 Compatible Imports (pandas-dev#23940)

3319221

Pingviinituutti pushed a commit to Pingviinituutti/pandas that referenced this pull request Feb 28, 2019

CLN: Cython Py2/3 Compatible Imports (pandas-dev#23940)

97aacc0

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CLN: Cython Py2/3 Compatible Imports #23940

CLN: Cython Py2/3 Compatible Imports #23940

WillAyd commented Nov 27, 2018 •

edited

Loading

pep8speaks commented Nov 27, 2018

WillAyd commented Nov 27, 2018

jbrockmendel Nov 27, 2018

jbrockmendel Nov 27, 2018

WillAyd Nov 27, 2018

jbrockmendel commented Nov 27, 2018

WillAyd commented Nov 27, 2018

jorisvandenbossche commented Nov 27, 2018

WillAyd commented Nov 28, 2018

WillAyd commented Nov 28, 2018

codecov bot commented Dec 1, 2018

codecov bot commented Dec 1, 2018 •

edited

Loading

jbrockmendel commented Dec 2, 2018

jreback left a comment

jreback Dec 3, 2018

jreback Dec 3, 2018

WillAyd commented Dec 6, 2018

jreback commented Dec 23, 2018

WillAyd commented Dec 25, 2018

jreback commented Dec 26, 2018

WillAyd commented Dec 26, 2018

jreback commented Dec 26, 2018

CLN: Cython Py2/3 Compatible Imports #23940

CLN: Cython Py2/3 Compatible Imports #23940

Conversation

WillAyd commented Nov 27, 2018 • edited Loading

pep8speaks commented Nov 27, 2018

WillAyd commented Nov 27, 2018

jbrockmendel Nov 27, 2018

Choose a reason for hiding this comment

jbrockmendel Nov 27, 2018

Choose a reason for hiding this comment

WillAyd Nov 27, 2018

Choose a reason for hiding this comment

jbrockmendel commented Nov 27, 2018

WillAyd commented Nov 27, 2018

jorisvandenbossche commented Nov 27, 2018

WillAyd commented Nov 28, 2018

WillAyd commented Nov 28, 2018

codecov bot commented Dec 1, 2018

Codecov Report

codecov bot commented Dec 1, 2018 • edited Loading

Codecov Report

jbrockmendel commented Dec 2, 2018

jreback left a comment

Choose a reason for hiding this comment

jreback Dec 3, 2018

Choose a reason for hiding this comment

jreback Dec 3, 2018

Choose a reason for hiding this comment

WillAyd commented Dec 6, 2018

jreback commented Dec 23, 2018

WillAyd commented Dec 25, 2018

jreback commented Dec 26, 2018

WillAyd commented Dec 26, 2018

jreback commented Dec 26, 2018

WillAyd commented Nov 27, 2018 •

edited

Loading

codecov bot commented Dec 1, 2018 •

edited

Loading