BrandonFors/pandas-github-issues huggingface dataset:
Convert DataFrame.rename to keyword only; simplify axis validation
State: closed

- [X] closes #29136 and ref #17963
- [X] tests added / passed
- [X] passes `black pandas`
- [X] passes `git diff upstream/master -u -- "*.py" | flake8 --diff`
- [X] whatsnew entry

I think we can greatly simplify core if we update our assumptions around the axes of dimensions we deal with. I came to this change trying to get rid of `NDFrame._construct_axes_from_arguments` which appears to do some argument mutation that isn't very clear. This was one of the few methods that called that, while also calling `validate_axis_style_args` which is guilty of the same kind of mutation

By being more explicit about the signature and converting this to keyword only arguments (which a FutureWarning detailed anyway) we can simplify the code and I think make more readable

cc @TomAugspurger 

Labels:
name: Clean

Comments:
Hello @WillAyd! 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-01-09 17:06:47 UTC
Closing to clear queue; will pick back up when I get a little more free time
Re-open?  This looks like a nice bugfix
Yea sure I'll try to take another look
@WillAyd conflict and some comments
@TomAugspurger yea that is the one case where this would break things, but the flip side is that also invites ambiguous uses like `df.rename(str.lower, index=str.upper)` (on master silently discards `index`) so not sure there's a perfect solution
Mmm I'd forgotten that case. So +1 to whatever the group thinks here :)
Planning to merge this in an hour, just before tagging the RC.
Thanks @WillAyd!
CLN: remove Block._try_coerce_arg
State: closed

Made possible by modernizing `libindex.convert_scalar`


Labels:
name: Internals
description: Related to non-user accessible pandas implementation
name: Clean

Comments:
The test failure here seems related? You might reproduce it locally by elevating warnings to errors with `pytest -Werror ...`
nice removals, pls create an issue / PR for the follow when you have a chance.
Improve performance of equality comparison between a simple Index and a MultiIndex
State: closed

Previously, Index.equals(MultiIndex) and MultiIndex.equals(Index) both involved converting the MultiIndex into an ndarray. Since this conversion requires resolving MultiIndex's by-reference structure (i.e. its codes + levels) into ndarray's by-value structure, it can be substantially computationally expensive.

However, it is only possible for a simple Index to equal a MultiIndex in two cases:

- the MultiIndex has only `1` level
- the MultiIndex has `d` levels, and the Index is an object index of size-`d` sequences (e.g. `d`-tuples)

Thus, if the Index is not object-typed, and its nlevels differs from that of the MultiIndex, then the two are determined to be unequal without ndarray conversion.

MWE:
```python
import pandas as pd

long_cheap_index = pd.RangeIndex(1000000)
short_expensive_index = pd.IntervalIndex(
    [pd.Interval(pd.Timestamp(2018, 10, 1), pd.Timestamp(2018, 10, 2))])

large_expensive_multiindex = pd.MultiIndex.from_product(
    [long_cheap_index, short_expensive_index])
trivial_simple_index = pd.Int64Index([])

# These operations no longer convert large_expensive_multiindex into an ndarray
# Previously, each took ~10 s; now, ~0.02 ms
large_expensive_multiindex.equals(trivial_simple_index)
trivial_simple_index.equals(large_expensive_multiindex)
```


- [x] tests added / passed
- [x] passes `black pandas`
- [x] passes `git diff upstream/master -u -- "*.py" | flake8 --diff`


Labels:
name: Performance
description: Memory or execution speed performance
name: MultiIndex

Comments:
@tlaytongoogle can you update, this looks good.
Hello @tlaytongoogle! 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-05 16:56:31 UTC
thanks @tlaytongoogle very nice!
Reduce Circular Imports with pandas.core.reshape.concat
State: closed

While working on #29124 I found a lot of general uses for this, but had to keep importing in the function body to prevent circular imports. By switching the isinstance checks to using ABC classes and moving the import of Index objects those circular imports can *mostly* be done away with

The only thing left that was causing circular imports was in the `Categorical` space. Can try to figure that out later, though cc @TomAugspurger in case that's a path that has already been gone down

Labels:
name: Refactor
description: Internal refactoring of code

Comments:
pending green, LGTM
> Can try to figure that out later, though cc @TomAugspurger in case that's a path that has already been gone down

I don't recall anyone trying it before.
FYI not sure if anyone has seen this but here are the official Python docs on dealing with circular imports:

https://docs.python.org/3/faq/programming.html#how-can-i-have-modules-that-mutually-import-each-other

We tend to violate Guido's recommendations a lot (ex: doing `from pandas.core.base import PandasObject` instead of `import pandas.core.base as base ... base.PandasObject` but also defining class variables that aren't built ins or constant values.

Something to consider...
@WillAyd can you re-push.  For some reason re-starting azure builds doesn't always take
needs rebase.  the azure failure hopefully unrelated
Yea still looking at this locally but trying to get a more comprehensive fix in, which could include categorical. The problem is that this module can spur a lot of imports from the index area which import from pandas.core.base which import from pandas.core.indexes ... so a lot of the circular imports I think can be traced back to that. Might have a few pre-cursor PRs to make this a full fledged change 
> which could include categorical.

that would be really nice if you can pull it off
> which could include categorical.

I'm looking at some related refactoring\*, wondering what you have in mind for Categorical, in particular if `.categories` would cease to be an Index.

\* Roughly, trying to get core.algorithms, core.nanops, and maybe core.missing to not depend on Index/Series etal
> I'm looking at some related refactoring*, wondering what you have in mind for Categorical, in particular if `.categories` would cease to be an Index.

Nothing concrete. Categorical *might* even be a red herring and the import of ExtensionArray from `pandas.core.arrays` really be the problem, as `pandas.core.arrays` imports things like Categorical before base is fully defined

I'm looking at this off and on so if it's something you are fully interested I certainly don't mind if you are motived to take over some aspects of this
Closing to clear queue; will pick back up when I get a little more free time
Make color validation more forgiving
State: closed

The current version throws a false positive if the style string contains `s` or `o`, which are valid marker styles and not colors.  For example:

```
style='s', color='C3'
```

should be legal, but currently throws an error.

My suggestion is to check for only the letters that are color codes.

- [ ] closes #xxxx
- [ ] tests added / passed
- [ ] passes `black pandas`
- [ ] passes `git diff upstream/master -u -- "*.py" | flake8 --diff`
- [ ] whatsnew entry


Labels:
name: Visualization
description: plotting

Comments:
Can you add a test for this? I'm not super familiar with this code but the intent of the ValueError getting raises also doesn't seem to match what you are asking for here, so test would help clarify that
I know that in theory it is best for the person submitting a PR to write tests, but I have almost no familiarity with the code base, so it would take me much longer than someone who knows the code, and I am more likely to break something.
But I can clarify the test case.  This example:
```
style='s', color='C3'
```
Should be legal because the style is specifying a shape, not a color.

Instead it raises "Cannot pass 'style' string with a color symbol and 'color' keyword argument.

The line of code I'm changing checks whether the style string contains any letters `a-z`, seemingly on the assumption that all letters in a style string are color codes.  But there are several letters, including `s` and `o`, that indicate shapes, not colors.
You can look at the tests defined in pandas/tests/plotting. Both test_series.py and test_frame.py should have examples you can copy; the color validation tests may be the best
Whoops, I've done something horribly wrong to your branch @AllenDowney. Will fix it up.
Ahh, I see. This was against 0.25.x. All our PRs go through master first.
thank you!

On Tue, Nov 5, 2019, at 1:24 PM, Tom Augspurger wrote:
> *@TomAugspurger* approved this pull request.

> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub <https://github.com/pandas-dev/pandas/pull/29122?email_source=notifications&email_token=AAOLP3MAOEMKLLHZRBR3G6LQSG27JA5CNFSM4JCWT4T2YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCKL6JAQ#pullrequestreview-311944322>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAOLP3JRCRT6Z3VB6TLS3C3QSG27JANCNFSM4JCWT4TQ>.

Thanks Allen!
The following test is wrong!
```
def test_style_single_ok(self):
    s = pd.Series([1, 2])
    ax = s.plot(style="s", color="C3")
    assert ax.lines[0].get_color() == ["C3"]
```
The color of a single Line2D cannot usually be a list. Perhaps a list of the 3 RGB or the 4 RGBa values. But not `["C3"]`. I'll change this test in PR #33821 .
mypy Fix for tests\frame\test_constructors
State: closed

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


Labels:

Comments:
thanks @happilyeverafter95 for the PR, however overlaps with #29115 which is now merged so closing.
Fix typing errors
State: closed

- [x] closes subtask of #28926
- [x] tests 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, @AbhijeetKrishnan !
Thanks!

> On Oct 21, 2019, at 11:04 AM, Abhijeet Krishnan <notifications@github.com> wrote:
> 
> @AbhijeetKrishnan commented on this pull request.
> 
> In pandas/tests/dtypes/test_inference.py <https://github.com/pandas-dev/pandas/pull/29114#discussion_r337160476>:
> 
> > @@ -288,7 +288,10 @@ class MockFile:
>      assert not is_file(data)
>  
>  
> -@pytest.mark.parametrize("ll", [collections.namedtuple("Test", list("abc"))(1, 2, 3)])
> +test_tuple = collections.namedtuple("Test", ["a", "b", "c"])
> Opened two issues with typeshed for
> 
> incorrect type inference on collections.namedtuple(): python/typeshed#3394 (comment) <https://github.com/python/typeshed/issues/3394#issue-510165256>
> incorrect type inference for literal list('abc'): python/typeshed#3395 (comment) <https://github.com/python/typeshed/issues/3395#issue-510169457>
> —
> You are receiving this because you commented.
> Reply to this email directly, view it on GitHub <https://github.com/pandas-dev/pandas/pull/29114?email_source=notifications&email_token=AAEU4UKKXJ64HNKSN5V6R7TQPXVLHA5CNFSM4JCVC5I2YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCIVBHYY#discussion_r337160476>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAEU4UNRM5W5BBAR6QLPX6TQPXVLHANCNFSM4JCVC5IQ>.
> 

William Ayd
william.ayd@icloud.com
DOC: Add instructions how to activate virtual env under windows with pip
State: closed

Contributing.rst now correctly describes how to setup dev environment
under Windows when using pip and not conda.

Closes #29112

- [x] closes #29112 
- [x] tests added / passed
- [x] passes `black pandas`
- [x] passes `git diff upstream/master -u -- "*.py" | flake8 --diff`
- [x] whatsnew entry


Labels:
name: Docs

Comments:
Hi @WillAyd ,
I've adjusted the comments as discussed above, please take a look when it is convenient to you

Regards
@datapythonista mind taking a look here?
Alright I need to apologize - yesterday I was tired and I've committed some brain fart to the repo.
What was missing was the information about the cmd.exe. Under Windows you can use two shells (WSL is a whole other story) - cmd.exe and powershell and they behave quite differently.

Now I've added the info about cmd.exe in the comments in the Windows code-block. Also I did some more testing with powershell and I've adjusted the initial generation of the virtual env, so that it 100% functions.

I've also added the `--no-use-pep517` argument to the Windows instructions, because it seems that under Windows a pep517 build gets triggered always. At least on the three different machines, I've tested with. Please see #28633
Hello, I did the requested changes

Regards
Thanks @gbaychev
Fix mypy errors tests.dtypes.test_common
State: closed

xref #28926

- [x] xref #28926
- [x] tests 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:
@AbhijeetKrishnan can you rebase
@simonjayhawkins Thanks for the review. I will try to see how the code might be refactored to appease mypy. I plan on submitting another PR if I fix it, so this PR can be closed.
> I plan on submitting another PR if I fix it, so this PR can be closed.

no need. make updates on same branch and push to keep history.
@AbhijeetKrishnan can you rebase
@AbhijeetKrishnan can you rebase, this looks about ready
Looks like this has now been addressed in https://github.com/pandas-dev/pandas/pull/29179 thanks all the same @AbhijeetKrishnan
Fix mypy errors tests.series.test_constructors
State: closed

xref #28926

- [x] closes a subtask of #28296
- [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 @AbhijeetKrishnan
BUG: IndexError in libreduction
State: closed

cc @WillAyd @jreback another from the groupby-Exception follow-ups.

Labels:
name: Clean

Comments:
Any easy way to summarize how this was throwing an IndexError? Not immediately apparent
> Any easy way to summarize how this was throwing an IndexError? Not immediately apparent

Case where `self.bins` is empty, so `self.bins[0]` raises `IndexError`.  The `self.ngroups > 0` check is incorrect because of a kludge on L217-221
does this affect user facing code in any way?
> does this affect user facing code in any way?

Not AFAICT, no.
can you rebase
Closing for now, I want to take a look at whats happening before we get here
BUG: fix TypeError raised in maybe_downcast_numeric
State: closed

Part of the groupby-Exception whittling; cc @WillAyd @jreback

Labels:
name: Bug
name: Dtype Conversions
description: Unexpected or buggy dtype conversions

Comments:
does this affect user facing code?
> does this affect user facing code?

Not really.  I've found one other place in groupby code where we can remove `except TypeError` once this is fixed, but thats about it
this looks reasonable
[#22550] Remove TestData from series-tests test_rank.py
State: closed

Part of #22550

* Replaced TestData usage in `pandas/tests/series/test_rank.py` with fixtures
* Switched from class-based test approach to function-based (**Note:** This is why the diff is so big)

- [x] tests added / passed
- [x] passes `black pandas`
- [x] passes `git diff upstream/master -u -- "*.py" | flake8 --diff`


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

Comments:
> * Switched from class-based test approach to function-based (**Note:** This is why the diff is so big)

Thanks for the PR. in future avoid this in the same PR as making changes. can always be done in a follow-up.
Thanks for your review @simonjayhawkins!

I took care of your first two comments. Do you want me to create a new issue for the other comment you made about parametrisation?
> I took care of your first two comments. Do you want me to create a new issue for the other comment you made about parametrisation?

see what @jreback has to say.

if the switch from class-based test approach to function-based is done as a follow-up instead of here, the additional fixtures won't be necessary here for the task of removing the TestData base class. in this case the parameterisation of the three tests that use the class variables could be done as a precursor to the switch.
Gotcha. Just let me know if I should revert the class to function switch - it won't be a lot of effort and I wouldn't mind.

On another note: I'll probably also take care of replacing TestData for the other series modules over the coming days. Should I continue creating one PR per module or is it fine to group the remaining ones in a single PR (possibly doing 1 commit per file)?
After this one, there are 5 more files pending
@simonjayhawkins @jreback I undid the class- to function-based transition to separate concerns and aid the review process.

Would be great if one of you could also reply to my previous comment. Cheers!
> On another note: I'll probably also take care of replacing TestData for the other series modules over the coming days. Should I continue creating one PR per module or is it fine to group the remaining ones in a single PR (possibly doing 1 commit per file)?
> After this one, there are 5 more files pending

creating one PR per module is fine.
@jreback Can this one be merged as well? I took care of all review comments
thanks @SaturnFromTitan
BUG: fix AttributeError raised in libreduction
State: closed

Bite-sized chunk of the promised "addressing of recently-added TODOs" for groupby Exception un-suppression.  cc @WillAyd @jreback.

Labels:
name: Bug
name: Groupby
name: Clean

Comments:
lgtm.
Looks like we have "slow" tests that still raise AttributeError.  AFAICT within the groupby/reduce machinery a `DatetimeIndex` is having `dti._data` temporarily overwritten with an ndarray (instead of DatetimeArray which it should be).  I expect this is related to the _index_data workaround in libreduction.

For now I'm reverting the re-raising of AttributeError and adding the index_data thing to my list of groupby-followups.
lgtm as well. 

> Looks like we have "slow" tests that still raise AttributeError.

Hmm do we just not hit these at all in CI?
> Hmm do we just not hit these at all in CI?

It's hit in the CI, but it wasn't hit locally before I opened the PR.  Currently working on a branch that I _think_ fixes the remaining AttributeError, but longer-term the _index_data thing is pretty nasty.
Shouldn't this add a test, or remove an otherwise catched exception?
> or remove an otherwise catched exception?

There is one more case where an AttributeError gets raised, so we can't just yet.
But the cause of the regression in the case I reported was that the AttributeError was no longer catched? Or for the case in our current tests, that's then probably still catched elsewhere?
> But the cause of the regression in the case I reported was that the AttributeError was no longer catched?

The thing that we recently stopped catching (or more specifically, started re-raising) consistently is AssertionError.  Without knowing anything about the geopandas case, this is my best guess for what might have been affected.

> Or for the case in our current tests, that's then probably still catched elsewhere?

If you look at the most recent commit for this PR, you'll see that in the status quo (and in this PR, after the last commit), AttributeError is caught in an `except Exception` block on L266.  Does that answer the question?
My example was running into another try/except block (see the one I linked in https://github.com/pandas-dev/pandas/issues/29141#issue-510309548), so I suppose that is simply not covered by our tests.

The reason it was working is because `_python_agg_general` is doing a `self._try_cast` on the result, casting back to the extension dtype if possible:

https://github.com/pandas-dev/pandas/blob/6c898e6a58285c841511e853bb1bf8a90766df78/pandas/core/groupby/groupby.py#L893-L905
The branch that fixes the remaining AttributeError (and so starts re-raising all AttributeErrors) is passing locally, will make a PR for that as a follow-up once this goes in.
thanks
BUG: parse_time_string failing to raise TypeError
State: closed

Cleans up one of the TODOs introduced in recent groupby work.

Adds some type annotations along the way.

Labels:
name: Clean

Comments:
lgtm. ping on green.

side issue, can you fix the allowed failure test on travis that is an html test failure (xfail or remove the test).
Ping

Will look at the Travis thing, likely Monday
thxs
TST: Add a regression test for the timezone issue
State: closed

Add a regression test to ensure that the timezone doesn't get removed
when using `df.apply`.

- [x] closes #29052
- [x] tests added / passed
- [x] passes `black pandas`
- [x] passes `git diff upstream/master -u -- "*.py" | flake8 --diff`


Labels:
name: Testing
description: pandas testing functions or related to the test suite
name: Timezones
description: Timezone data dtype
name: Apply
description: Apply, Aggregate, Transform, Map

Comments:
Hello @GrigoriosGiann! 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-10-20 10:52:05 UTC
> @GrigoriosGiann Thanks for the PR.

Thanks for the feedback! 😄
Let me know if there is anything else I should adjust. 🙂
lgtm. ping on green.
Azure appears to be stuck...
Is there a way to rerun checks?
Or should I just `git commit --amend --no-edit` & re-push?

Edit: Force re-pushed since azure failed after some time (https://dev.azure.com/pandas-dev/pandas/_build/results?buildId=19252&view=logs)
Ping, it's green!😄 
thanks @GrigoriosGiann
[#22550] Remove TestData from series-tests test_quantile.py
State: closed

Part of #22550

Replaced TestData usage in `pandas/tests/series/test_quantile.py` with `datetime_series` fixture

- [x] tests added / passed
- [x] passes `black pandas`
- [x] passes `git diff upstream/master -u -- "*.py" | flake8 --diff`


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

Comments:
Thanks @SaturnFromTitan 
As a follow-on could remove class.
TST: Add regression test for Series index.map failing (#20990)
State: closed

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


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

Comments:
@WillAyd ping
pls merge master
Hello @Reksbril! 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-11-18 15:55:56 UTC
Thanks @Reksbril !
[#22550] Remove TestData from series-tests test_operators.py
State: closed

Part of #22550

Replaced TestData usage in `pandas/tests/series/test_operators.py` with`datetime_series` fixture

- [x] tests added / passed
- [x] passes `black pandas`
- [x] passes `git diff upstream/master -u -- "*.py" | flake8 --diff`


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

Comments:
restarted travis build
LGTM
thanks @SaturnFromTitan