Accept constant memoryviews in HashTable.lookup #21688

xhochy · 2018-06-30T07:32:55Z

building an ExtensionArray for nullable ints using a pyarrow.Array, the underlying memoryview is marked as constant. The HashTable Cython code uses the memoryview in the lookup function but in a read-only fashion. Without the const specifier Cython raises an error that the memoryview is read-only.

Not sure how to test this in a small scope (i.e. without using pyarrow) or if this small change is worth mentioning in the changelog.

tests added / passed
passes git diff upstream/master -u -- "*.py" | flake8 --diff
whatsnew entry

xhochy · 2018-06-30T10:20:08Z

The Travis failure seems quite silent. Is there a way to extract more information?

TomAugspurger · 2018-06-30T18:33:52Z

I restarted the one. I haven't seen that error before.

codecov · 2018-06-30T18:59:23Z

Codecov Report

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

@@            Coverage Diff             @@
##           master   #21688      +/-   ##
==========================================
- Coverage   91.95%   91.95%   -0.01%     
==========================================
  Files         160      160              
  Lines       49837    49820      -17     
==========================================
- Hits        45830    45812      -18     
- Misses       4007     4008       +1

Flag	Coverage Δ
#multiple	`90.33% <ø> (-0.01%)`	⬇️
#single	`42.07% <ø> (-0.01%)`	⬇️

Impacted Files	Coverage Δ
pandas/core/indexes/category.py	`97.01% <0%> (-0.28%)`	⬇️
pandas/core/arrays/datetimelike.py	`94.38% <0%> (-0.25%)`	⬇️
pandas/core/indexes/period.py	`93.35% <0%> (-0.23%)`	⬇️
pandas/core/indexes/multi.py	`94.96% <0%> (-0.09%)`	⬇️
pandas/core/groupby/groupby.py	`92.65% <0%> (-0.05%)`	⬇️
pandas/core/indexes/timedeltas.py	`91% <0%> (-0.03%)`	⬇️
pandas/core/frame.py	`97.19% <0%> (-0.01%)`	⬇️
pandas/core/strings.py	`98.63% <0%> (ø)`	⬆️
pandas/core/arrays/datetimes.py	`100% <0%> (ø)`	⬆️
pandas/util/testing.py	`85.33% <0%> (ø)`	⬆️
... and 6 more

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 e84cf2a...a4cb629. Read the comment docs.

TomAugspurger · 2018-06-30T19:35:57Z

Must have been a one-off failure.

cc @chris-b1 if you have any thoughts on this?

jreback · 2018-06-30T19:46:05Z

pandas/_libs/hashtable_class_helper.pxi.in

@@ -379,7 +379,7 @@ cdef class {{name}}HashTable(HashTable):
                self.table.vals[k] = i

    @cython.boundscheck(False)
-    def lookup(self, {{dtype}}_t[:] values):
+    def lookup(self, const {{dtype}}_t[:] values):


we have multiple lookup routines for different drapes - is there a reason to not change them all?

why just lookup? we have many routines which couldnin theory benefit from this

I can make a pass over hashtable_class_helper.pxi.in and add const specifiers where applicable.

chris-b1 · 2018-06-30T19:55:29Z

Now that cython supports const memoryviews we probably should go ahead and use them everywhere possible, #18825 example of working around this.

Looks like would bump our required version up to 0.28 but shouldn't really be a problem since we distribute c files on PyPI?
https://github.com/cython/cython/blob/master/CHANGES.rst#028-2018-03-13

xhochy · 2018-06-30T20:26:56Z

doc/source/whatsnew/v0.24.0.txt

@@ -343,6 +343,8 @@ Other
 ^^^^^

 - :meth: `~pandas.io.formats.style.Styler.background_gradient` now takes a ``text_color_threshold`` parameter to automatically lighten the text color based on the luminance of the background color. This improves readability with dark background colors without the need to limit the background colormap range. (:issue:`21258`)
+- Building pandas for development now requires ``cython >= 0.28``
+- Require at least 0.28 version of cython to support read-only memoryviews (:issue:`21688`)


Referenced the PR as issue here. Is that sufficient or would you like to have separate issues for each line for documentation purposes?

so we really need to bump cython here?

Assuming we need to bump Cython, it looks like there are a few things missing in order to fully bump it: pin some CI builds to the minimum Cython version, update version number in install.rst.

See the changes in #18623 for where I bumped to 0.24; should show all the places that need updating.

Reading through the changelog of Cython, we need to bump to 0.28 to support the const specifier for memoryviews. Looking a bit through the Travis setup, we only pin Cython for the 2.7 builds and these are the ones that failed. (sadly with a weird error message).

we need to bump to 0.28 to support the const specifier for memoryviews

is this an actual change in cython?

I agree its nice to change this, but cython 0.28 is pretty new.

can you move the building pandas to a new sub-section (like we did previously when we bumped deps, as going to do this for more deps this cycle).

also is 0.28 min, or 0.28.2?

0.28.0 works but we should build with 0.28.2 due to the mentioned problems in sklearn. I would at least bump the Cython version on Travis & CricleCI.

Should we then allow 0.28.0 or require 0.28.2 so that we don't get the bug reports w.r.t. problems that are fixes in .2?

jreback · 2018-07-01T14:19:19Z

doc/source/whatsnew/v0.24.0.txt

@@ -343,6 +343,8 @@ Other
 ^^^^^

 - :meth: `~pandas.io.formats.style.Styler.background_gradient` now takes a ``text_color_threshold`` parameter to automatically lighten the text color based on the luminance of the background color. This improves readability with dark background colors without the need to limit the background colormap range. (:issue:`21258`)
+- Building pandas for development now requires ``cython >= 0.28``
+- Require at least 0.28 version of cython to support read-only memoryviews (:issue:`21688`)


so we really need to bump cython here?

jorisvandenbossche · 2018-07-02T12:11:18Z

is this an actual change in cython?

Yes, it is a new cython feature that it can now work on read-only memoryviews (see the third bullet point here: https://github.com/cython/cython/blob/master/CHANGES.rst#028-2018-03-13). This is also a feature much wanted by sklearn: scikit-learn/scikit-learn#10624

As @chris-b1 said, if we bump the cython version requirement, we should try to simplify our code base and use memoryviews in more places (#10070 as another example where two version of a function are added, one with memoryviews and other with ndarray interface to support read-only arrays)

IMO, ideally, if we bump the version, it would be good to test for a few internal cases that the const memoryviews are actually working (since this is new functionality in cython, there might be still some rough edges. In sklearn they found a bug in const memoryviews that was fixed in 0.28.2).
(but since I am not going to do that work, no strong opinion on doing this before actually bumping the version requirement)

TomAugspurger · 2018-07-02T12:14:56Z

Looks like would bump our required version up to 0.28 but shouldn't really be a problem since we distribute c files on PyPI?

That's correct.

xhochy · 2018-07-03T13:38:12Z

Happy to implement the necessary changes when we bump the Cython version but it would be nice to know whether this is the preferred way forward?

jreback · 2018-07-03T13:57:54Z

yeah am ok with bumping the cython version i guess for 0.24

since this is not user facing really

we should also again bump numpy min but that’s a different issue

jreback · 2018-07-07T14:26:26Z

doc/source/whatsnew/v0.24.0.txt

@@ -343,6 +343,8 @@ Other
 ^^^^^

 - :meth: `~pandas.io.formats.style.Styler.background_gradient` now takes a ``text_color_threshold`` parameter to automatically lighten the text color based on the luminance of the background color. This improves readability with dark background colors without the need to limit the background colormap range. (:issue:`21258`)
+- Building pandas for development now requires ``cython >= 0.28``
+- Require at least 0.28 version of cython to support read-only memoryviews (:issue:`21688`)


can you move the building pandas to a new sub-section (like we did previously when we bumped deps, as going to do this for more deps this cycle).

also is 0.28 min, or 0.28.2?

jreback · 2018-07-07T14:28:13Z

pandas/tests/test_algos.py

@@ -1079,13 +1079,17 @@ class TestHashTable(object):

    def test_lookup_nan(self):


can you make this a fixture and use this for False/True

jreback · 2018-07-07T14:33:36Z

after we merge this can try to remove the other read-only memory fixes that are in take routines
xrref #10070 and #12813

jreback · 2018-07-07T15:20:20Z

pushed some small doc changes. ping on green for merge.

jreback · 2018-07-07T19:52:45Z

thanks @xhochy nice patch!

Backported from pandas-devgh-21688.

* CI: Bump NumPy to 1.9.3 Backport of gh-22499. * BLD: Fix openpyxl to 2.5.5 Backport of gh-22601. * CI: Resolve timeout issue on Travis Backported from gh-22429. * CI: Migrate to CircleCI 2.0 Backport of gh-21814. * Upgrade Cython to >=0.28.2 Backported from gh-21688. * TST: Patch locale handling Backported from gh-21739. Backport of gh-22213.

TomAugspurger added the Algos Non-arithmetic algos: value_counts, factorize, sorting, isin, clip, shift, diff label Jun 30, 2018

jreback requested changes Jun 30, 2018

View reviewed changes

xhochy commented Jun 30, 2018

View reviewed changes

jreback requested changes Jul 1, 2018

View reviewed changes

jreback added the Dependencies Required and optional dependencies label Jul 2, 2018

xhochy force-pushed the accept-constant-memoryview-lookup branch from 6d38e08 to 36626d8 Compare July 7, 2018 13:17

jreback requested changes Jul 7, 2018

View reviewed changes

xhochy added 10 commits July 7, 2018 17:04

Accept constant memoryviews in HashTable.lookup

475e99d

Add const specifiers to remaining signatures

bcd1384

Increase minimal Cython version to 0.28

081067c

Add whatsnew section

e42a718

Add tests for readonly memoryviews

bf4eed7

Bump Cython version in CI to 0.28

6c9341b

Apply modern fix for pandas-dev#18825

6c335b0

Start new section 'Build Changes' in whatsnew

9deaf07

Test writable arrays with parameters

e0b7098

Bump CI version for Cython to 0.28.2

20c181f

xhochy force-pushed the accept-constant-memoryview-lookup branch from 36626d8 to 20c181f Compare July 7, 2018 15:04

jreback added 2 commits July 7, 2018 10:16

add writeable fixture & fix docs

0552d61

bump min version

64fd445

jreback added this to the 0.24.0 milestone Jul 7, 2018

jreback approved these changes Jul 7, 2018

View reviewed changes

jreback mentioned this pull request Jul 7, 2018

Factorize fails with read-only array #12813

Closed

add min cython versions everywhere

a4cb629

jreback merged commit dcbf8b5 into pandas-dev:master Jul 7, 2018

xhochy deleted the accept-constant-memoryview-lookup branch July 8, 2018 18:21

gfyoung added a commit to forking-repos/pandas that referenced this pull request Sep 10, 2018

Upgrade Cython to >=0.28.2

530a21f

Backported from pandas-devgh-21688.

gfyoung mentioned this pull request Sep 11, 2018

CI / BLD: Various CI Backports #22637

Merged

Sup3rGeo pushed a commit to Sup3rGeo/pandas that referenced this pull request Oct 1, 2018

Accept constant memoryviews in HashTable.lookup (pandas-dev#21688)

2fae688

xhochy mentioned this pull request Oct 2, 2018

ARROW-3392: [Python] Support filters in disjunctive normal form in ParquetDataset apache/arrow#2677

Closed

xhochy mentioned this pull request Oct 22, 2018

Error "buffer source array is read-only" when using unique() on df loaded with feather #23276

Closed

maartenbreddels mentioned this pull request Apr 23, 2019

fix: pandas has issues with read only buffers, this is a workaround vaexio/vaex#226

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Accept constant memoryviews in HashTable.lookup #21688

Accept constant memoryviews in HashTable.lookup #21688

xhochy commented Jun 30, 2018 •

edited by jreback

Loading

xhochy commented Jun 30, 2018

TomAugspurger commented Jun 30, 2018

codecov bot commented Jun 30, 2018 •

edited

Loading

TomAugspurger commented Jun 30, 2018

jreback Jun 30, 2018

xhochy Jun 30, 2018

chris-b1 commented Jun 30, 2018

xhochy Jun 30, 2018

jreback Jul 1, 2018

jschendel Jul 1, 2018 •

edited

Loading

xhochy Jul 1, 2018

jreback Jul 2, 2018

jreback Jul 7, 2018

xhochy Jul 7, 2018

jreback Jul 1, 2018

jorisvandenbossche commented Jul 2, 2018

TomAugspurger commented Jul 2, 2018

xhochy commented Jul 3, 2018

jreback commented Jul 3, 2018

jreback Jul 7, 2018

jreback Jul 7, 2018

jreback commented Jul 7, 2018

jreback commented Jul 7, 2018

jreback commented Jul 7, 2018

		@@ -1079,13 +1079,17 @@ class TestHashTable(object):

		def test_lookup_nan(self):

Accept constant memoryviews in HashTable.lookup #21688

Accept constant memoryviews in HashTable.lookup #21688

Conversation

xhochy commented Jun 30, 2018 • edited by jreback Loading

xhochy commented Jun 30, 2018

TomAugspurger commented Jun 30, 2018

codecov bot commented Jun 30, 2018 • edited Loading

Codecov Report

TomAugspurger commented Jun 30, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chris-b1 commented Jun 30, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jschendel Jul 1, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jorisvandenbossche commented Jul 2, 2018

TomAugspurger commented Jul 2, 2018

xhochy commented Jul 3, 2018

jreback commented Jul 3, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jreback commented Jul 7, 2018

jreback commented Jul 7, 2018

jreback commented Jul 7, 2018

xhochy commented Jun 30, 2018 •

edited by jreback

Loading

codecov bot commented Jun 30, 2018 •

edited

Loading

jschendel Jul 1, 2018 •

edited

Loading