BrandonFors/pandas-github-issues huggingface dataset:
BUG: strengthen typing in get_c_string, fix StringHashTable segfault
State: closed

Start with util.pxd, we can tighten the arg from "object" to "str" and simplify it a bit.  Tracking down the places where get_c_string is used, the main one is in StringHashTable, where in `set_item` it is currently used incorrectly.  The test added by this PR segfaults in master.

@TomAugspurger it looks like StringHashTable has relatively light testing.  Should we be using it for StringArray?

Labels:
name: Strings
description: String extension data type and string data
name: Clean

Comments:
lgtm. not sure why the 36 build is failing though.
> not sure why the 36 build is failing though.

Looks like we're passing `np.str_` instead of `str` somewhere.  Haven't looked into it closer than that so far (kinda hoping we bump numpy minversion and it fixes itself)
So it isnt a numpy-version problem, just a slow test that is only getting run in that build.  We're passing a np.str_ object instead of just a str.  This is tough to check for because `isinstance(some_np_str, str)` is True
lgtm.
thanks @jbrockmendel
Fixing pandas/tests/indexes/multi/test_analytics.py to utilize python…
State: closed

Modified reflect python3 format strings on pandas/tests/indexes/multi/test_analytics.py.  I ran the test and it passed like before.  ref [#29886](https://github.com/pandas-dev/pandas/issues/29886) and [#29547](https://github.com/pandas-dev/pandas/issues/29547).

Labels:
name: Code Style
description: Code style, linting, code_checks
name: Clean

Comments:
>Should fix [#29886](https://github.com/pandas-dev/pandas/issues/29886) and [#29547](https://github.com/pandas-dev/pandas/issues/29547).

@AncientRickles can you please replace the word ```fix``` with the word ```ref```?

Just so it wouldn't close the issue.
thanks @AncientRickles
TYP: Typing annotations pandas/util/
State: closed

- [ ] closes #xxxx
- [x] tests added / passed
- [x] passes `black pandas`
- [x] passes `git diff upstream/master -u -- "*.py" | flake8 --diff`
- [ ] whatsnew entry


Labels:
name: Typing
description: type annotations, mypy/pyright type checking

Comments:
thanks @MomIsBestFriend
CLN: str.format -> f-strings for `io/sas`
State: closed

- [x] ref #29547 
- [ ] tests added / passed
- [x] passes `black pandas`
- [x] passes `git diff upstream/master -u -- "*.py" | flake8 --diff`
- [ ] whatsnew entry


Labels:
name: Code Style
description: Code style, linting, code_checks

Comments:
@jyscao: Thanks for the contribution, and welcome to `pandas` !  🚀 

LGTM.  Codecov failure can be ignored.


>     * [x]  closes #29547


Can you please change the word ```closes``` to ```ref``` ?
restarted the travis build.  LGTM.
Thanks @jyscao
CLN: Old string formatting: .format() -> f
State: closed

- [ ] ref #29547
- [ ] tests added / passed
- [ ] passes `black pandas`
- [ ] passes `git diff upstream/master -u -- "*.py" | flake8 --diff`
- [ ] whatsnew entry


Labels:
name: Code Style
description: Code style, linting, code_checks

Comments:
Hello @DorAmram! Thanks for updating this PR. We checked the lines you've touched for [PEP 8](https://www.python.org/dev/peps/pep-0008) issues, and found:



There are currently no PEP 8 issues detected in this Pull Request. Cheers! :beers: 

##### Comment last updated at 2019-12-22 18:03:07 UTC
Hmm I think this file is autogenerated as per @jbrockmendel comments here https://github.com/pandas-dev/pandas/pull/29518#discussion_r344826266
yeah we can leave this one alone, do we have a check for .format usage in checks? (prob not yet)
> yeah we can leave this one alone, do we have a check for .format usage in checks? (prob not yet)

Not yet there are still 200+ files to tackle. 
`grep -l -R -e '%s' -e '%d' -e '\.format(' --include=*.{py,pyx} pandas/`

If we want to add a check now we can do - and users remove the file from exclude list once done?
(Would help confirm all instances of .format have been removed in the PR for a file)

grepping for `.format(` will be complicated by the fact that we have 6 places where we `def format\(`.  But we can worry about that later.  This PR is non-actionable, closing.
WIP/ DOC: Move 'For Developers' content from wiki to contributing docs. #30232
State: closed


Labels:
name: Docs

Comments:
Thanks @souvik3333, I think moving things from the wiki here is a good idea, but looks like the page you moved is not relevant anymore, now that we don't support Python 2.

Closing, but we should remove the page from the wiki (I don't have permissions, or may be it's just that it's not possible in the mobile version). 
> Can you explain why you added so much content about Python 2 compatibility?

I was working on issue #30232 and this content was mentioned there. @gfyoung @datapythonista Can you verify the other content of the list in the issue description?
https://github.com/pandas-dev/pandas/issues/30410 @souvik3333 there is some discussion going on about it here 
> #30410 @souvik3333 there is some discussion going on about it here

Yes, I just found it. Thank you.
> Thanks @souvik3333, I think moving things from the wiki here is a good idea, but looks like the page you moved is not relevant anymore, now that we don't support Python 2.

I think we want the `Imports (aim for absolute)` section and maybe more.
@souvik3333 I think the additions here should be added to doc/source/development/contributing_code_guide.rst once #30129 is merged
Closing this, since all the Python 2/3 documentation is not useful anymore. @souvik3333 as said, once #30129 is merged, you can add a section about the imports there. Thanks!
ENH: Add ignore_index for df.drop_duplicates
State: closed

- [x] xref #30114 
- [x] tests added / passed
- [x] passes `black pandas`
- [x] passes `git diff upstream/master -u -- "*.py" | flake8 --diff`
- [x] whatsnew entry


Labels:
name: Reshaping
description: Concat, Merge/Join, Stack/Unstack, Explode
name: API Design

Comments:
thanks @charlesdong1991
TYP: Type annotations in pandas/io/formats/printing.py
State: closed

- [ ] closes #xxxx
- [x] tests added / passed
- [x] passes `black pandas`
- [x] passes `git diff upstream/master -u -- "*.py" | flake8 --diff`
- [ ] whatsnew entry


Labels:
name: Typing
description: type annotations, mypy/pyright type checking

Comments:
Couple of comments, generally looks good.  I'm not sure, but it _may_ be the case that since py2 was dropped some usages of pprint_thing throughout the code base are now unnecessary
thanks @MomIsBestFriend
TYP: Type annotations in pandas/io/formats/style.py
State: closed

- [ ] closes #xxxx
- [x] tests added / passed
- [x] passes `black pandas`
- [x] passes `git diff upstream/master -u -- "*.py" | flake8 --diff`
- [ ] whatsnew entry


Labels:
name: Typing
description: type annotations, mypy/pyright type checking

Comments:
Thank you for the code suggestions @WillAyd 

needs a rebase
Was this good to go? @simonjayhawkins I think there was a question for you in https://github.com/pandas-dev/pandas/pull/30403#discussion_r361183658, if you happen to know the answer.
@TomAugspurger sorry missed that. Have responded but could always be a follow-on. IMO better to do in several passes than in one go. force-pushes make it hard to continually review and typing can be gradual.
> force-pushes make it hard to continually review and typing can be gradual.

Noted
PLEASE DO NOT MERGE, after the rebase, to https://github.com/pandas-dev/pandas/commit/9f97d114113a59590319bcd411e1132c3026d758 mypy is raising errors. posibly also related to https://github.com/pandas-dev/pandas/issues/30449
The errors were fixed.
> (think I commented on all)

You have :)


Rebased

--- 

(unrelated but important)

@WillAyd have you saw what I wrote [here](https://github.com/pandas-dev/pandas/pull/31091#issuecomment-575698301)?
Thanks @MomIsBestFriend
ENH: Add ignore_index for df.sort_values and series.sort_values
State: closed

- [x] xref #30114 
- [x] tests added / passed
- [x] passes `black pandas`
- [x] passes `git diff upstream/master -u -- "*.py" | flake8 --diff`
- [x] whatsnew entry

This is the first step for closing the PR #30114, I will come up with another PR for `drop_duplicates`

Labels:
name: Reshaping
description: Concat, Merge/Join, Stack/Unstack, Explode
name: API Design

Comments:
thanks @charlesdong1991
TYP: Type hints in pandas/io/formats/excel.py
State: closed

- [ ] closes #xxxx
- [x] tests added / passed
- [x] passes `black pandas`
- [x] passes `git diff upstream/master -u -- "*.py" | flake8 --diff`
- [ ] whatsnew entry


Labels:
name: Typing
description: type annotations, mypy/pyright type checking

Comments:
Hello @MomIsBestFriend! Thanks for updating this PR. We checked the lines you've touched for [PEP 8](https://www.python.org/dev/peps/pep-0008) issues, and found:



There are currently no PEP 8 issues detected in this Pull Request. Cheers! :beers: 

##### Comment last updated at 2019-12-29 14:59:34 UTC
lgtm. over to you @WillAyd 
Thanks @MomIsBestFriend for this and all the others
TYP: Typing hints in pandas/io/formats/{css,csvs}.py
State: closed

- [ ] closes #xxxx
- [x] tests added / passed
- [x] passes `black pandas`
- [x] passes `git diff upstream/master -u -- "*.py" | flake8 --diff`
- [ ] whatsnew entry


Labels:
name: Typing
description: type annotations, mypy/pyright type checking

Comments:
@simonjayhawkins care to look?
thanks @MomIsBestFriend
STY: Underscores for long numbers
State: closed

- [ ] closes #xxxx
- [ ] tests added / passed
- [x] passes `black pandas`
- [x] passes `git diff upstream/master -u -- "*.py" | flake8 --diff`
- [ ] whatsnew entry


Labels:
name: Code Style
description: Code style, linting, code_checks

Comments:
Hello @MomIsBestFriend! Thanks for updating this PR. We checked the lines you've touched for [PEP 8](https://www.python.org/dev/peps/pep-0008) issues, and found:



There are currently no PEP 8 issues detected in this Pull Request. Cheers! :beers: 

##### Comment last updated at 2019-12-22 12:18:12 UTC
Thanks, @MomIsBestFriend
REF: use fused types for part of hashtables code
State: closed

This implements fused-type functions for the hashtable code that currently uses tempita.  Same idea as #29244, but this breaks off a part that I can confirm is performance-neutral.

Labels:
name: Refactor
description: Internal refactoring of code
name: Internals
description: Related to non-user accessible pandas implementation

Comments:
I’m having trouble getting the next step to be performant, and it’s not worth doing this without that.  So closing.
TST: Added fromisocalendar test cases
State: closed

- [x] closes #28115
- [x] tests added / passed
- [x] passes `black pandas`
- [x] passes `git diff upstream/master -u -- "*.py" | flake8 --diff`
- [ ] whatsnew entry


Labels:
name: Datetime
description: Datetime data dtype
name: Compat
description: pandas objects compatability with Numpy or Python functions

Comments:
cc @jbrockmendel 
This is a good start.  Next up you need to add tests for the new functionality.  Those will go in `pandas.tests.scalar.timestamp.test_timestamp.TestTimestampConstructors`.  Look at the tests we have there for the other similar Timestamp classmethods to get an idea of what the tests should look like
if you look at the CI  results you'll see some red checkmarks.  you're going to need to click through to find out why this is failing so you can fix it (spoiler: test_missing_public_nat_methods)
Good going so far.  You changed it to raise in older python versions and fixed the test that was broken before.  But now if you look at the CI you'll see there is a different test that is broken.

So the next step is to look at the CI log to tell what test(s) are failing (spoiler: the test you added is failing when run in older pythons) and figure out how to fix it.  (Ideally also you'll run the tests locally to make sure they pass on your computer before pushing, let me know if you have trouble with that)

This becomes more intuitive with experience.
>Ideally also you'll run the tests locally to make sure they pass on your computer before pushing

I did, the problem is I have python 3.8

Anyway, I'll fix it.


Is there's  more elegant way rather than making an ```if``` statement checking the python version?
Good progress
So I just tried pd.Timestamp.fromisocalendar on 0.25.3 and it works fine (i.e. correctly returns a Timestamp).  I think we need a test for this, but the actual implementation in the cython file is unnecessary.
> So I just tried pd.Timestamp.fromisocalendar on 0.25.3 and it works fine (i.e. correctly returns a Timestamp). I think we need a test for this, but the actual implementation in the cython file is unnecessary.

So remove the code here?

https://github.com/MomIsBestFriend/pandas/blob/88468fa3604457319401b126764ed49be3e6d15f/pandas/_libs/tslibs/timestamps.pyx#L340

(Removed the new created method?)
> (Removed the new created method?)

I think the test will still pass, so yes.
Since I really didn't add anything new to the code base (except test cases).

Can #28115 be closed?
> Can #28115 be closed?

Yes it can
@jbrockmendel do we need to do anything for NaT here? add make_error_func and test for it?
> do we need to do anything for NaT here? add make_error_func and test for it?

Yes.  ATM it looks like `pd.NaT.fromisocalendar(y, m, d)` incorrectly calls `NaT.__new__` and raises a non-helpful TypeError.  @MomIsBestFriend can you update and test NaT appropriately?
@MomIsBestFriend seems like there are some CI failures for py37 and earlier.
code & tests lgtm. can you add a whatsnew node; other enhancements section is ok, just say added compatiblity with datetime.isocalender for PY >= 38
thanks @MomIsBestFriend
CLN: tslibs.parsing
State: closed

- [x] closes #22234

Non-segfault-related pieces broken off from #30374.  Fix a (not-reachable-but-thats-another-conversation) tzinfo check closing #22234.

De-duplicates get_rule_month in tslibs.frequencies and tslibs.parsing

pythonizes a PyUnicode_Join usage because cython does that optimization already.

Labels:
name: Timezones
description: Timezone data dtype
name: Clean

Comments:
lgtm to me; @WillAyd  & @mroeschke make some good points.
Thanks @jbrockmendel
CI: transition powershell-> bash
State: closed

xref #26344

If this works, i think the remaining places to bashify are
- identical powerhsell commands in windows.yml and posix.yml to check for test failures
- conda.recipe/bld.bat (do we need this directory at all?)
- test.bat, test_fast.bat (do we need the bash versions of these?)

Labels:
name: Build
description: Library building on various platforms
name: CI
description: Continuous Integration

Comments:
Thanks @jbrockmendel 
@jreback any idea if we need conda.recipe or test/test_fast files mentioned in the OP?  (or binstar.yml, that looks you-specific)
binstar is really old can remove

the test_fast are local scripts
need a canonical way to run all tests
how do u usually do it?
 how do u usually do it?

In my .bash_aliases file i have

```
gitd () {
	BRANCH=$1
	rm -rf ../$BRANCH
	git worktree prune
	git branch -D $BRANCH
}

pbranch () {
	BRANCH=$1
	git branch $BRANCH
	git worktree add ../$BRANCH $BRANCH
	cd ..
	cd $BRANCH
	python3 setup.py build_ext --inplace
}

alias ptest="python3 -m pytest pandas/tests --skip-slow --skip-db"
alias pflake="git diff --name-only | grep '\.py$' | xargs flake8"
alias psort="git diff --name-only | grep '\.py$' | xargs isort"
alias pblack="git diff --name-only | grep '\.py$' | xargs black"
alias pbuild="python3 setup.py build_ext --inplace"
alias ppull="git pull upstream master"

```
>  need a canonical way to run all tests

if we want to support this robustly, we might want to consider going back to `tox`
@jbrockmendel the one disadvantage to your commands versus pytest would be the lack of parallel execution (i.e. `-n 8` flag). Indifferent on getting rid of the file but if we do probably worth specifying the parallel execution ability as it can save significant time when running tests
so would be nice to push some of these commands to the makefile:)

also not averse to having. single canonical test_fast.sh (which we basically have now)
TYP: Annotations in core/indexes/
State: closed

- [ ] closes #xxxx
- [ ] tests added / passed
- [x] passes `black pandas`
- [x] passes `git diff upstream/master -u -- "*.py" | flake8 --diff`
- [ ] whatsnew entry


Labels:
name: Code Style
description: Code style, linting, code_checks

Comments:
Just the one typo, ping on green
ping @jbrockmendel 
Thanks @MomIsBestFriend
CLN/WIP: use conditional nogil
State: closed

This is largely an experiment to see how much code we could de-duplicate once cython 0.30.0 comes out and conditional `with nogil` becomes available.  Its a decent amount, less than I'd expected.

Hopefully this can help cython kick the tires.

Labels:
name: Refactor
description: Internal refactoring of code
name: Internals
description: Related to non-user accessible pandas implementation

Comments:
@WillAyd anywhere else I missed where we can rip out code using conditional nogil (zero hurry).  I'd be interested in your thoughts on the apparently-untested TIEBREAK_FIRST/object code path.

@pilkibun IIRC #27373 was a proof of concept for things we could clean up once cython 0.30.0 is out.  Any other suggestions?
Still interested in thoughts from @WillAyd and @pilkibun (see previous comment), but this PR was just a proof of concept, so closing.
Yep sorry; great idea but admittedly didn't put a ton into review - waiting for Cython to actually release first
@WillAyd the part that I thought you'd be interested in is that we don't have any tests that go through the TIEBREAK_FIRST/object path.  I'm hoping we can either a) un-disallow that and not raise mid-loop or b) raise up-front pre-loop.  Pinging you on it is based on a vague recollection that you had worked on it before.
Hmm maybe I just forgot but don't recall touching the algos code (did something similar in groupby)  so maybe don't fully grasp the context but from glancing at it your suggestion to raise outside of the loop seems logical
DEPR: Deprecate pandas.np module
State: closed

- [x] closes #30296 
- [x] tests added / passed
- [x] passes `black pandas`
- [x] passes `git diff upstream/master -u -- "*.py" | flake8 --diff`
- [x] whatsnew entry



Labels:
name: Deprecate
description: Functionality to remove in pandas

Comments:
Hello @lithomas1! Thanks for updating this PR. We checked the lines you've touched for [PEP 8](https://www.python.org/dev/peps/pep-0008) issues, and found:









There are currently no PEP 8 issues detected in this Pull Request. Cheers! :beers: 

##### Comment last updated at 2019-12-26 02:00:49 UTC
> py36 needs to be handled separately

You can still do the `import numpy as np` in the `else` clause.

Can you also add tests for this? (that it still works but generates a warning for py37)
@jbrockmendel and @jorisvandenbossche 
Can you take another look at the code? I added tests and made the deprecation warnings also show on Python 3.6 
ping @jreback 
CI has passed. 
thanks @lithomas1 very nice!
REF: targeted test files for nlargest, searchsorted, value_counts
State: closed

In parallel with, and orthogonal to, #30381.

Identified a couple of tests that were in the wrong places, e.g. the one moved int test_cumulative.

Labels:
name: Refactor
description: Internal refactoring of code
name: Testing
description: pandas testing functions or related to the test suite

Comments:
thanks @jbrockmendel
Remove unnecessary `@td.skip_if_no("pathlib")`
State: closed

- [x] closes #30375
- [ ] tests added / passed
- [x] passes `black pandas`
- [x] passes `git diff upstream/master -u -- "*.py" | flake8 --diff`
- [ ] whatsnew entry


Labels:

Comments:
thanks but the patch for this issue was recently merged (1 hour ago)
BUG: Series.drop() with MultiIndex: inconsistent behaviour
State: closed

- [x] closes #8594
- [x] tests added / passed
- [x] passes `black pandas`
- [x] passes `git diff upstream/master -u -- "*.py" | flake8 --diff`
- [x] whatsnew entry

description:
In core.base 
```
        indexer = self.get_indexer(labels)
        mask = indexer == -1
        if mask.any():
            if errors != "ignore":
                raise KeyError(f"{labels[mask]} not found in axis")
```
single index check and raise if there is no matching indexer. But 'MultiIndex' case,
In indexes.multi
```
        codes = com.index_labels_to_array(codes)
        i = self._get_level_number(level)
        index = self.levels[i]
        values = index.get_indexer(codes)

        mask = ~algos.isin(self.codes[i], values)

        return self[mask]
```
no check. So i add check logic and raise exception if there is no matching indexer.
This bug cause from 'MultiIndex', so do not distinguish between "Series" and "DataFrame"


Labels:
name: Bug
name: API Design
name: MultiIndex

Comments:
also needs a rebase
thanks @proost
[CLN] remove now-unnecessary td.skip_if_no(pathlib)
State: closed

- [ ] closes #30375 
- [ ] tests added / passed
- [ ] passes `black pandas`
- [ ] passes `git diff upstream/master -u -- "*.py" | flake8 --diff`
- [ ] whatsnew entry


Labels:
name: Testing
description: pandas testing functions or related to the test suite
name: Clean

Comments:
@simonjayhawkins sure, done - thanks for your review, and sorry for not having picked up on that straight away
thanks @MarcoGorelli
Clean Up nogil Warning From Parsers
State: closed

I think just easier to ensure The GIL is acquired within the C function rather than all of the current casting / indirection.

Eliminates the following warning:

```sh
warning: pandas/_libs/parsers.pyx:1656:34: Casting a GIL-requiring function into a nogil function circumvents GIL validation
```

Labels:
name: Clean

Comments:
presumably the nogil version was for performance.  does that turn out not to matter?
Nogil is still in play here; the one function that requires it just explicitly grabs it now
Ran the csv benchmarks and they showed no change.
@jreback all green
thanks @WillAyd
Removed unsigned parametrization from Intervaltree
State: closed

closes #30365 , #27169

Unclear if we currently consider this a feature, but I couldn't find anything explicitly asking for it so trying the removal route first

Not sure what the other alternative would be for parametrization, but I think generally we need to be careful when doing comparisons between signed and unsigned in templating or fused types

Side benefit - this gets rid of 128 build warnings and a slight perf boost to compilation


Labels:
name: Interval
description: Interval data type

Comments:
> this gets rid of 128 build warnings

That'd be really nice
Hmm missed the parametrized tests that are causing failures. Suppose this won't be as easy as I'd hoped
@jschendel just saw your comment here which I think is applicable:

https://github.com/pandas-dev/pandas/pull/20651#issuecomment-381046318

From quite a while ago but does this still ring a bell? Do you know what fix you had that raised an `OverflowError`? I actually think that would be preferable?
I don't remember exactly what I did to get the `OverflowError` but my vague recollection is that I had to create two additional fused types for signed vs unsigned and them add them as an additional parameter to `nodes` here:

https://github.com/pandas-dev/pandas/blob/5a7b5c958c6f004c395136f9438a1ed6c04861dd/pandas/_libs/intervaltree.pxi.in#L233-L235

And then appropriately use the signed/unsigned fused types within the node classes.

I recall trying a lot of different things, so the above might not actually be the appropriate thing to do, and is really just the thing I most distinctly recall trying.
@jschendel already fixed this. closing
CLN: replace str.format() with f-string
State: closed

- [x] ref #29547
- [ ] tests added / passed
- [x] passes `black pandas`
- [x] passes `git diff upstream/master -u -- "*.py" | flake8 --diff`
- [ ] whatsnew entry


Labels:
name: Code Style
description: Code style, linting, code_checks

Comments:
Great thanks @AlpAribal !