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.

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

BUG: Hashtable size hint cap #22805

Merged
merged 7 commits into from Jan 15, 2019

Conversation

Projects
None yet
5 participants
@chris-b1
Copy link
Contributor

commented Sep 22, 2018

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

Chris Bartak and others added some commits Sep 21, 2018

@pep8speaks

This comment has been minimized.

Copy link

commented Sep 22, 2018

Hello @chris-b1! Thanks for updating the PR.

Line 1419:45: E261 at least two spaces before inline comment

Comment last updated on January 14, 2019 at 20:09 Hours UTC
@@ -266,6 +266,7 @@ cdef class {{name}}HashTable(HashTable):
def __cinit__(self, size_hint=1):
self.table = kh_init_{{dtype}}()
if size_hint is not None:
size_hint = min(size_hint, _SIZE_HINT_LIMIT)

This comment has been minimized.

Copy link
@jreback

jreback Sep 23, 2018

Contributor

you can type the size_hint as a uint_t I think to avoid this problem

This comment has been minimized.

Copy link
@chris-b1

chris-b1 Dec 13, 2018

Author Contributor

It's getting typed as a uint on the next line - we actually want it untyped here so it could accept arbitrarily large values, just needs capped before before being converted to the c-type (what was causing the original error)

@jreback

This comment has been minimized.

Copy link
Contributor

commented Nov 23, 2018

can you rebase

@jreback

This comment has been minimized.

Copy link
Contributor

commented Dec 12, 2018

@chris-b1 can you rebase / update

@codecov

This comment has been minimized.

Copy link

commented Dec 12, 2018

Codecov Report

Merging #22805 into master will decrease coverage by 0.03%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #22805      +/-   ##
==========================================
- Coverage   92.21%   92.18%   -0.04%     
==========================================
  Files         162      169       +7     
  Lines       51768    50810     -958     
==========================================
- Hits        47739    46839     -900     
+ Misses       4029     3971      -58
Flag Coverage Δ
#multiple 90.6% <ø> (-0.02%) ⬇️
#single 42.37% <ø> (-0.63%) ⬇️
Impacted Files Coverage Δ
pandas/io/formats/html.py 90.68% <0%> (-7.93%) ⬇️
pandas/core/arrays/period.py 91.98% <0%> (-6.31%) ⬇️
pandas/plotting/_tools.py 72.62% <0%> (-6.15%) ⬇️
pandas/io/feather_format.py 85.71% <0%> (-4.03%) ⬇️
pandas/io/parquet.py 73.72% <0%> (-3.2%) ⬇️
pandas/core/arrays/base.py 94.24% <0%> (-3.18%) ⬇️
pandas/plotting/_style.py 74.28% <0%> (-3.14%) ⬇️
pandas/core/sparse/scipy_sparse.py 97.05% <0%> (-2.95%) ⬇️
pandas/core/arrays/datetimes.py 96.82% <0%> (-1.74%) ⬇️
pandas/core/indexes/range.py 95.73% <0%> (-1.6%) ⬇️
... and 136 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 9bc42f9...74b3b45. Read the comment docs.

@codecov

This comment has been minimized.

Copy link

commented Dec 12, 2018

Codecov Report

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

Impacted file tree graph

@@           Coverage Diff           @@
##           master   #22805   +/-   ##
=======================================
  Coverage   92.38%   92.38%           
=======================================
  Files         166      166           
  Lines       52363    52363           
=======================================
  Hits        48377    48377           
  Misses       3986     3986
Flag Coverage Δ
#multiple 90.81% <ø> (ø) ⬆️
#single 42.91% <ø> (ø) ⬆️

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 ff40c2f...d3cbb2b. Read the comment docs.

Chris Bartak
@chris-b1

This comment has been minimized.

Copy link
Contributor Author

commented Dec 13, 2018

@jreback - I think this is good now

@@ -573,9 +574,10 @@ cdef class StringHashTable(HashTable):
# or a sentinel np.nan / None missing value
na_string_sentinel = '__nan__'

def __init__(self, int size_hint=1):
def __init__(self, size_hint=1):

This comment has been minimized.

Copy link
@jreback

jreback Dec 13, 2018

Contributor

can you just type this as int64?

This comment has been minimized.

Copy link
@chris-b1

chris-b1 Dec 13, 2018

Author Contributor

good point, that'll work

Chris Bartak
@mroeschke

This comment has been minimized.

Copy link
Member

commented Dec 31, 2018

This just needs a rebase and it looks good to go (Travis failure was a timeout)

@jreback

This comment has been minimized.

Copy link
Contributor

commented Jan 1, 2019

can you merge master and update

@jreback

This comment has been minimized.

Copy link
Contributor

commented Jan 14, 2019

@chris-b1 can you merge master

@chris-b1

This comment has been minimized.

Copy link
Contributor Author

commented Jan 14, 2019

@jreback - should be good now!

@jreback jreback added this to the 0.24.0 milestone Jan 15, 2019

@jreback jreback merged commit f512a61 into pandas-dev:master Jan 15, 2019

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
pandas-dev.pandas Build #20190114.29 succeeded
Details
@jreback

This comment has been minimized.

Copy link
Contributor

commented Jan 15, 2019

thanks @chris-b1

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

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.