BrandonFors/pandas-github-issues huggingface dataset:
DOC: update ohlc docstring so that it reflects the real use #31919
State: closed

- [x] closes #31919
- [x] tests added / passed

The test python scripts/validate_docstrings.py pandas.core.groupby.GroupBy.ohlc returns errors, but that is something that happens with many functions at the moment

```################################################################################
################# Docstring (pandas.core.groupby.GroupBy.ohlc) #################
################################################################################

Compute open, high, low and close values of a group, excluding missing values.

For multiple groupings, the result index will be a MultiIndex

Returns
-------
DataFrame
    Open, high, low and close values within each group.

See Also
--------
Series.groupby
DataFrame.groupby

################################################################################
################################## Validation ##################################
################################################################################

3 Errors found:
	Missing description for See Also "Series.groupby" reference
	Missing description for See Also "DataFrame.groupby" reference
	No examples section found```

Labels:
name: Docs

Comments:
> but that is something that happens with many functions at the moment

Yes, the CI check ignores certain errors:
```
### DOCSTRINGS ###
if [[ -z "$CHECK" || "$CHECK" == "docstrings" ]]; then

    MSG='Validate docstrings (GL03, GL04, GL05, GL06, GL07, GL09, GL10, SS04, SS05, PR03, PR04, PR05, PR10, EX04, RT01, RT04, RT05, SA02, SA03, SA05)' ; echo $MSG
    $BASE_DIR/scripts/validate_docstrings.py --format=actions --errors=GL03,GL04,GL05,GL06,GL07,GL09,GL10,SS04,SS05,PR03,PR04,PR05,PR10,EX04,RT01,RT04,RT05,SA02,SA03,SA05
    RET=$(($RET + $?)) ; echo $MSG "DONE"

fi
```
Can you merge master?
> Can you merge master?

I don't have write access
> > Can you merge master?
> 
> I don't have write access

I think Will was suggesting that you merge them locally and push another commit:
```
git fetch upstream master
git merge upstream/master
git push origin ohlc-docstring-update
```

EDIT
----
There aren't any conflicts with master - @WillAyd was the purpose of the merge just to restart the CI process?
> Can you merge master?

Done
Thanks @dequadras
REF: use iloc instead of _ixs outside of indexing code
State: closed

Readers are much more likely to be familiar with iloc than _ixs.

This leaves us with exactly one usage of _ixs.

Labels:
name: Indexing
description: Related to indexing on series/frames, not to indexes themselves
name: Clean

Comments:
> `_ixs` is considerably faster though (3-4x based on a quick test), so we should maybe not blindly change the one in the other

does this actually matter? more idiomatic code is *much* more preferred
I'd say it depends on the performance impact for real-world use cases, balanced against the cost of maintain each. I'm fine with having & using private indexing methods that are can use information known in the calling context to take dedicated paths, which isn't an option in general methods like `.iloc`.
> balanced against the cost of maintain each

I've been doing indexing stuff almost exclusively for weeks and still have to double-check what some of the internal methods do.

> depends on the performance impact

Side-note: we can optimize iloc a bit skipping some of the validation and letting numpy's exceptions bubble up.  Thoughts?
Performance matters. So unless it is shown that this has no significant impact on an actual use case, I would not just replace it. 
IMHO, `_ixs` is a rather simple, easy to understand function that provides a fastpath to get a single positional column or row. If it is not directly clear what it is doing from the name when reading the code (the name certainly isn't clear), then let's rename it?

> we can optimize iloc a bit skipping some of the validation and letting numpy's exceptions bubble up. Thoughts?

It's certainly useful to optimize iloc I think, but then I would do it as a precursor (or in this PR)

@jorisvandenbossche can you share how you got to those benchmarks? I think would be helpful to frame discussion
I just compared `.iloc[:, i]` with `._ixs(i, axis=1)` on a dummy dataframe. 
ive got some optimizations for iloc in the pipeline, will revisit after those.  closing.
dont skip keyerror for IntervalIndex
State: closed

cc @jschendel this is speculative that this is the desired behavior.

This is our only use of `Index.is_interval()`

Labels:
name: Indexing
description: Related to indexing on series/frames, not to indexes themselves
name: Error Reporting
description: Incorrect or improved errors from pandas
name: Interval
description: Interval data type

Comments:
no tests changed? or just running a previously skipped a skipped test? rebase, lgtm.
> no tests changed?

No tests affected.  Found this while trying to shave off corner cases in indexing, and it isn't clear that this is needed.
lgtm. merge on green.
CLN: 29547 replace old string formatting 2
State: closed

I splitted PR #31844 in batches, this is the second
For this PR I ran the command `grep -l -R -e '%s' -e '%d' -e '\.format(' --include=*.{py,pyx} pandas/` and checked all the files that were returned for `.format(` and changed the old string format for the corresponding `fstrings` to attempt a full clean of, [#29547](https://github.com/pandas-dev/pandas/issues/29547). I may have missed something so is a good idea to double check just in case

- [ x  ] tests added / passed
- [ x ] passes `black pandas`
- [ x ] passes `git diff upstream/master -u -- "*.py" | flake8 --diff`

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

Comments:
Great thanks @3vts
Revert 31791
State: closed

supersedes #31800

we never back ported this so just cleaning up master (assuming it now works again)

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

Comments:
one npdev fail left
thanks @WillAyd 
Why did we revert this? 

We are testing all external error messages, IMO there is no point in testing those so detailed (each time a package changes the wording slightly, our tests will start failing)
not all of these are external , they need to be separated into 2 lists
EHN: fix unsigned int type problem of the result diff()
State: closed

An unexpected output type(float64) occurs to diff() when using uint
type. So fix it by restoring original dtype after storing it

Fixes #28909

- [ ] closes #xxxx
- [ ] tests added / passed
- [ ] passes `black pandas`
- [ ] passes `git diff upstream/master -u -- "*.py" | flake8 --diff`
- [ ] whatsnew entry


What do you think about this problem with the code? @jreback 

Labels:

Comments:
For numeric types, `Series.diff()` introduces NaNs, which are a float. These can't be cast back to int / uint.

I'd recommend using one of the new nullable integer dtypes.
Thanks for your help.
CLN: Some groupby internals
State: closed

- [x] tests added / passed
- [x] passes `black pandas`
- [x] passes `git diff upstream/master -u -- "*.py" | flake8 --diff`

1. Avoids calling `_get_sorted_data()` twice
2. Eliminates `*args, **kwargs` from an internal function

Labels:
name: Groupby
name: Clean

Comments:
Looks like a segfault on travis - restarted to see
potential annotations; LGTM pending green
Can you merge master? Slightly concerned about reproducibility of previous segfault
Thanks for the review. Merged master and all green
CLN: 29547 replace old string formatting 1
State: closed

I splitted PR #31844 in batches, this is the first one
For this PR I ran the command `grep -l -R -e '%s' -e '%d' -e '\.format(' --include=*.{py,pyx} pandas/` and checked all the files that were returned for `.format(` and changed the old string format for the corresponding `fstrings` to attempt a full clean of, [#29547](https://github.com/pandas-dev/pandas/issues/29547). I may have missed something so is a good idea to double check just in case

- [ x  ] tests added / passed
- [ x ] passes `black pandas`
- [ x ] passes `git diff upstream/master -u -- "*.py" | flake8 --diff`


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

Comments:
lgtm outside of @MomIsBestFriend comment
Great thanks @3vts
CLN: implement _getitem_tuple_same_dim
State: closed

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


Labels:
name: Indexing
description: Related to indexing on series/frames, not to indexes themselves
name: Clean

Comments:
lgtm. @WillAyd comment seems worth it.
Thanks @jbrockmendel
BUG: Handle NA in assert_numpy_array_equal
State: closed

- [x] closes #31881
- [x] tests added / passed
- [x] passes `black pandas`
- [x] passes `git diff upstream/master -u -- "*.py" | flake8 --diff`

I think in the the case where we have NA we want to use identity rather than equality for checking when two arrays are equal.

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

Comments:
I think worth back porting to 1.0.2
> I think worth back porting to 1.0.2

Yes, it's internal only, but since other fixes that will be backported might want to rely on this testing functionality, let's backport
@dsaxton Thanks!
BUG: using loc[int] with object index
State: closed

- [x] closes #19456
- [x] tests added / passed
- [x] passes `black pandas`
- [x] passes `git diff upstream/master -u -- "*.py" | flake8 --diff`
- [ ] whatsnew entry

This causes us to raise KeyError instead of TypeError in a couple of places, consistent with #31867.

Also note this leaves us with only one non-plotting usage of `holds_integer`, and it wouldnt surprise me if that one is subtly causing problems too.

Labels:
name: Bug
name: Indexing
description: Related to indexing on series/frames, not to indexes themselves

Comments:
lgtm. merge on green.
DOC: Update sort_index docs
State: closed

- [x] closes #31880

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

Comments:
@WillAyd Good to merge or were there changes you'd like me to make? Not sure if there's a "nice" way to get around the mypy annoyance
Yea would be nice to get the typing figured out, but OK for another day
Thanks @dsaxton
BUG: fix length_of_indexer with boolean mask
State: closed

- [x] closes #25774
- [x] tests added / passed
- [x] passes `black pandas`
- [x] passes `git diff upstream/master -u -- "*.py" | flake8 --diff`
- [ ] whatsnew entry

Allows for a nice cleanup in _setitem_with_indexer.  Note this will conflict with #31887; the order doesn't matter.

Labels:
name: Indexing
description: Related to indexing on series/frames, not to indexes themselves

Comments:
> is this a bug fix? (the OP was)

Only sorta-kinda.  The current behavior is wrong (assuming im correctly interpreting the desired behavior as "give the expected length of obj[indexer]"), but AFAICT nothing user-facing.

gentle ping (for this and #31887).  i think ive got a handle on the brokenness of _setitem_with_indexer and would like to get these simplications in before ripping apart whats left
CLN: D202 No blank lines allowed after function docstring
State: closed

This one could conflict which the unchecked rule blank line before comment

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

Comments:
im fine with this either way, seems harmless
Can you merge master?
@MomIsBestFriend is there supposed to be a code check for this? Vaguely recall that but may be wrong
@WillAyd If I recall correctly in ```scripts/validate_doctrings.py``` there's a check for that, but it's not integrated in the CI due to the fact that there's too many occurrences, and they never got fixed (until now)

---

cc @datapythonista
Sounds like we might not need the custom code check with work that @simonjayhawkins is doing, so great!

Thanks!
CLN: D300 Use """triple double quotes"""
State: closed


Labels:
name: Clean

Comments:
looks good; is this a code that should be added to CI?
> looks good; is this a code that should be added to CI?

these are pydocstyle error codes, http://www.pydocstyle.org/en/5.0.2/error_codes.html. pydocstyle has support for numpy convention checks and a flake8 plugin. It is current;y used by matplotlib and there could possibly be collaboration between numpy and pydocstyle, see https://github.com/numpy/numpydoc/issues/213#issuecomment-544236897, for the static elements.

there is overlap and differences between validate_docstrings and pydocstyle and so both tools could complement each other. The flake8 plugin is handy for ide highlighting. the pydocstyle cli could be handy for ci.

at these stage just looking to formalise the clean-ups in #31162, #31462 and others. then we can look to lock-down.
Cool sounds good
CLN: simplify _setitem_with_indexer
State: closed

The MultiIndex check it is doing is unnecessary for positional indexing.

Other things being cleaned up here appear to be remnants from Panel

Labels:
name: Indexing
description: Related to indexing on series/frames, not to indexes themselves
name: Clean

Comments:
Now you have been working with `_setitem_with_indexer`, could you add a docstring for it explaining what the scope of this function is (eg only used for positional indexing?)
> could you add a docstring for it explaining what the scope of this function is (

good idea
im of mixed mind on adding a docstring at the moment, because there is a big gap between what the method does vs what it is supposed to do.
Even trying to write down a bit what you know about what it is supposed to do and what it actually does (what led you to previous comment) is useful IMO; it doesn't need to be a perfectly standard formatted docstring
lgtm needs a rebase
rebased+green
@jorisvandenbossche gentle ping
Thanks!
CLN: Move info
State: closed

precursor to xref #31796. Have moved `DataFrame.info` code into `pandas/io/formats/info.py`, along with tests

Labels:
name: Clean

Comments:
@MarcoGorelli can you merge master to resolve conflicts
@simonjayhawkins sure, done
Hello @MarcoGorelli! 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 2020-02-18 10:21:23 UTC
@jreback sure, have reverted renaming the fixture and have added type annotations
@jreback sure, have moved all imports to the top and have reverted moving the fixture
thanks @MarcoGorelli 

move PRs have to be exactly a move only (and whatever fixes are needed in the target files  to get green CI), otherwise very difficult / impossible to reveiw.
DOC: add redirects from Rolling to rolling.Rolling
State: closed

- [x] closes #31762 

Labels:
name: Docs

Comments:
@jorisvandenbossche Can you please tell me whether it's correct or not? If it is then I will add other Rolling related redirects too.
@jorisvandenbossche Can you please check out the doc failures? It says `Could not import extension contributors (exception: No module named 'gitdb.utils.compat')`
The failures have gone. Can you take a look back, please? @jorisvandenbossche 
Still some problems with the CI. Can you merge master and see if it finishes successfully now please?
```
git fetch upstream
git merge upstream/master
git push
```
Merged master. Ping on green.
The doc build is passing, so merging this
Thanks @rushabh-v
CLN: f-string formatting
State: closed

Ref to [#29547](https://github.com/pandas-dev/pandas/issues/29547)

- [X] passes `black pandas`
- [X] passes `git diff upstream/master -u -- "*.py" | flake8 --diff`

Replacing strings interpolated with .format() with strings interpolated with f-strings

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

Comments:
Hello @monicaw218! 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 2020-02-11 04:36:39 UTC
Great thanks @monicaw218
API/BUG: raise only KeyError failed on geitem/loc lookups
State: closed

- [x] closes #21567
- [ ] tests added / passed
- [x] passes `black pandas`
- [x] passes `git diff upstream/master -u -- "*.py" | flake8 --diff`
- [ ] whatsnew entry

ATM we raise a mix of KeyError and TypeError.

I'm in the process of going through the Indexing issues will see what else this closes.

Labels:
name: Indexing
description: Related to indexing on series/frames, not to indexes themselves
name: Error Reporting
description: Incorrect or improved errors from pandas

Comments:
As a follow-up to this, it turns out we can entirely rip out _convert_scalar_indexer, which will be really nice
I think this makes sense though may break some user code catching a TypeError, so if others agree at least worth a note *maybe* a deprecation
I don't think it is possible to do with a deprecation? Or how would people be able to avoid the deprecation? 
We could maybe also have a custom exception class that inherits both ..

Apart of changing the API; I assume that those different errors where there initially to provide a more useful error message to the user (instead of just "KeyError: some_value") ?
ok I think this is reasonable, e.g. this highlites the difference between indexing a list for example with a non-integer (raises TypeError) and a dict with anything (raises KeyError). consistency is good. 

As far as user impact; this is a change, but likely does not affect very many downstream users (who are likely catching both TypeError and KeyError).

would be ok with simply changing this (to raise KeyError), unless other objections.
this is a good change, any objections @jorisvandenbossche 
Thanks @jbrockmendel
BUG: catch almost-null-slice in _convert_slice_indexer
State: closed

its a contrived corner case, but i wanted to use com.is_null_slice rather than duplicating it in _convert_slice_indexer

Labels:
name: Bug
name: Indexing
description: Related to indexing on series/frames, not to indexes themselves

Comments:
Reachable from user code?
> Reachable from user code?

Haven't found any examples so far.
rebase this just in case
rebased+green
thanks!
REF: move Loc method that belongs on Index
State: closed


Labels:
name: Refactor
description: Internal refactoring of code
name: Index
description: Related to the Index class or subclasses

Comments:
+/- 0 on this; I don't know that this is necessarily clearer as a index method but no major objections
@WillAyd gentle ping
I don't have a strong enough preference on this to go either way. @pandas-dev/pandas-core for other thoughts if a blocker
will have a look soon
this isnt a big deal, just a code smell that id like to get out of the way
+1: ideally ``if isinstance(labels, ABCMultiIndex):`` should never appear outside ``pandas/core/indexes/multi.py``. Even clearer would be to remove the need for (non-multi) ``Index`` to support this method (but I didn't check how complicated it is - and this PR is certainly good per se).
thanks, yeah getting Index/MI closer and closer but there will always be ovrrides.
> thanks, yeah getting Index/MI closer and closer but there will always be ovrrides.

Sure! I was just saying that ideally overrides should be only in MI code.
REF: make Series/DataFrame _slice always positional
State: closed

ATM we have a loc/iloc kwarg that NDFrame._slice (which is in effect DataFrame._slice) ignores and always slices positionally.  This makes Series._slice always slice positionally too.

Labels:
name: Indexing
description: Related to indexing on series/frames, not to indexes themselves
name: Clean

Comments:
Thanks @jbrockmendel
REF: Use nonzero in place of argwhere
State: closed

Use nonzero to preserve 1d of Series

xref #31813



Labels:
name: IO Stata
description: read_stata, to_stata
name: Clean

Comments:
I think this is a better fix @jbrockmendel .  `argwhere` should not have been used it seems.
LGTM pending green
Great thanks @bashtage
CLN-29547 replace old string formatting
State: closed


Labels:

Comments:
Thanks @3vts 

Looks like you need to run `black` against any Python file you've modified, see [contributing to the code base](https://pandas.pydata.org/pandas-docs/stable/development/contributing.html#contributing-to-the-code-base)
@3vts this PR is too big

keep the modifications to < 10 files as a time
@MarcoGorelli I did, the error came when I resolved conflicts
@jreback will do, sorry, new here.....
BUG: Fix raw parameter not being respected in groupby.rolling.apply
State: closed

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


Labels:
name: Bug
name: Window
description: rolling, ewma, expanding

Comments:
thanks @mroeschke 
@mroeschke can you check if this was backported?
Don't see it in https://github.com/pandas-dev/pandas/commits/1.0.x. I'll backport it when I get a chance
@MeeseeksDev backport to 1.0.x
@mroeschke I mentioned it on the core list as well now, but know that often commenting with what I did above is enough, which can avoid you need to take the time to manually backport.
Thanks @jorisvandenbossche. Sorry I forgot to revisit this and backport this PR
BUG: Fix rolling.corr with time frequency
State: closed

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


Labels:
name: Bug
name: Window
description: rolling, ewma, expanding

Comments:
@mroeschke can you rebase / merge on green.
@MeeseeksDev backport to 1.0.x

Owee, I'm MrMeeseeks, Look at me.

There seem to be a conflict, please backport manually. Here are approximate instructions:

1. Checkout backport branch and update it.

```
$ git checkout 1.0.x
$ git pull
```

2. Cherry pick the first parent branch of the this PR on top of the older branch:
```
$ git cherry-pick -m1 8dd9fabd2ad9104e747084437b9ad436d5be087a
```

3. You will likely have some merge/cherry-pick conflict here, fix them and commit:

```
$ git commit -am 'Backport PR #31841: BUG: Fix rolling.corr with time frequency'
```

4. Push to a named branch :

```
git push YOURFORK 1.0.x:auto-backport-of-pr-31841-on-1.0.x
```

5. Create a PR against branch 1.0.x, I would have named this PR:

> "Backport PR #31841 on branch 1.0.x"

And apply the correct labels and milestones.

Congratulation you did some good work ! Hopefully your backport PR will be tested by the continuous integration and merged soon!

If these instruction are inaccurate, feel free to [suggest an improvement](https://github.com/MeeseeksBox/MeeseeksDev).