BrandonFors/pandas-github-issues huggingface dataset:
ENH: Add suffixes argument for pd.concat
State: closed

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


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

Comments:
Thanks for the PR @charlesdong1991 ! Left a few comment above
thanks for your comment, @jreback 

I initially opened an issue #29615 for this, and later found you opened the issue already long ago. The usecase I am having right now is I have several tables with exactly same format, but they have different values in each field since let's say they are representing the data from different years, e.g

```
df1 = pd.DataFrame({"a": [1, 2, 3], "b": [2, 3, 4]})
df2 = pd.DataFrame({"a": [2, 3, 4], "b": [3, 4, 5]})
df3 = pd.DataFrame({"a": [4, 5, 6], "b": [6, 7, 8]})
```

And I would like to put them together horizontally so as to do further analysis. The easiest way in this case would be `pd.concat([df1, df2, df3], axis=1)`, but after concatenation, the dataframe contains duplicated column names, and it would be very useful and convenient to have a `suffixes` argument to distinguish those as `merge` has. And this is my initial motivation to create this PR.


Been thinking this over and while this is a nice effort I think I am -1 on adding this to the concat API. It feels intuitive for merge where you are essentially taking a union of columns, but with pd.concat you are just stacking things. 

Seems like if anything this de-duplication would really be a Index method (though not sure that’s worth adding to the API either)
Many thanks for taking your time and giving me very nice reviews @jschendel and I appreciate a lot!

However, since I see @WillAyd and @jreback have concerns if this feature should be added to `pd.concat` or not, and i agree that the change is mainly doing a de-duplication on column Index by adding a suffix since `pd.concat` is just stacking. The only small benefit of having this feature might be if having many tables with identical names, stacking them together while renaming with suffix can be easier.

I am open to advices on the opinions on if it's worth to have this enhancement, if it is, I will start code changes based on @jschendel 's nice reviews.
Thanks for the contribution @charlesdong1991 but based on feedback I don't think we have an appetite for this one
Extension Module Compat Cleanup
State: closed

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


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

Comments:
lgtm, pending green.
thanks
CLN: de-privatize names in core.computation
State: closed

Some annotations.  I'm finding that annotations in this directory are really fragile; adding a type to one thing will cause a mypy complaint in somewhere surprising.

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

Comments:
> While I'm not sure what the rationale was for de-privatizing

Adhering to the convention of respecting private names makes it easier to reason about the code
thanks
CLN:F-strings
State: closed

- [ ] closes #xxxx
- [x] tests added / passed
- [x] passes `black pandas`
- [x] passes `git diff upstream/master -u -- "*.py" | flake8 --diff`
- [ ] whatsnew entry

ref https://github.com/pandas-dev/pandas/issues/29547

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

Comments:
How this failed test related to any of my changed files?
Can any of the devs help me out?
thanks!
CI: Forcing GitHub actions to activate
State: closed

GitHub actions won't start running until the first action is committed.

This PR adds a simple action with an echo, so we can activate GitHub actions risk free, and when we move an actual build, the action will run in the PR, and we can see that it works.

Tested this action in a test repo, to make sure it works: https://github.com/datapythonista/xql/commit/cdc596df626da5f81980c773de94269546880753/checks?check_suite_id=314215608

Labels:
name: CI
description: Continuous Integration

Comments:
sure happy to check this out.
BUG: resolved problem with DataFrame.equals() (#28839)
State: closed

The function was returning True in case shown in added test. The cause
of the problem was sorting Blocks of DataFrame by type, and then
mgr_locs before comparison. It resulted in arranging the identical blocks
in the same way, which resulted in having the same two lists of blocks.
Changing sorting order to (mgr_locs, type) resolves the problem, while not
interrupting the other aspects of comparison.

- [x] closes #28839
- [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: Reshaping
description: Concat, Merge/Join, Stack/Unstack, Explode

Comments:
also merge master
thanks @Reksbril 
LGTM pending Will's comment
CI: Fix error when creating postgresql db
State: closed

- [X] closes #29643

Not sure if this makes sense, but let's try if this is the problem.

Labels:
name: CI
description: Continuous Integration

Comments:
Looks like this change adds this at the beginning of the log:
```
$ travis_setup_postgresql
Starting PostgreSQL v9.2
sudo service postgresql start 9.2
$ sudo service mysql start
start: Job is already running: mysql
```
But still the same error. I guess we'll have to add traces during the build to see if the postgres service is running, and what are the server, port...
maybe if we used 9.5 instead of 9.2?  the log has a bit about shutting down 9.5, then starting 9.4 then stopping 9.4, then starting 9.3...
Didn't see that, let's see if this works. I'm going to sleep now, feel free to push here if you want to try more things.
Maybe someone from @travis-ci can help us here?
I'm going to move the failing build to GitHub actions. That was more or less planned (#28545), and looks like shouldn't be difficult.
let’s just disable the server for now
until it can be diagnosed

> let’s just disable the server for now

Not sure what do you mean. Disable the failing build?

I guess if we temporary disable the creation of the db, and remove `psycopg2` from the dependencies, the CI will be green again, and just the postgres tests will be skipped.

But with some luck I can move the build to GitHub actions easily, and we can keep running the postgres tests, and have a permanent solution. I'll need #29661 merged soon for that.
I think the problem is that something in https://github.com/pandas-dev/pandas/blob/master/ci/deps/travis-36-cov.yaml is making conda install postgres, see:
```
postgresql                11.2                 h20c2e04_0
```
I didn't see that there are two travis builds where the db is correctly created, and I think this is the main difference. In the working builds postgres is not installed.

Let's see if uninstalling coda fixes the error.
@pandas-dev/pandas-core this fixes the problem with the CI (the postgresql error)
great merge one green
CI: Format isort output for azure
State: closed

- [x] closes #27179
- [x] tests added / passed
- [x] passes `black pandas`
- [x] passes `git diff upstream/master -u -- "*.py" | flake8 --diff`

Current behaviour on master of isort formatting:
![image](https://user-images.githubusercontent.com/16733618/69010035-12c41f80-0953-11ea-8e70-4d17e8dec943.png)


Finishing up stale PR @https://github.com/pandas-dev/pandas/pull/27334


Labels:
name: Error Reporting
description: Incorrect or improved errors from pandas
name: CI
description: Continuous Integration

Comments:
Since we are now using Github Actions ill update this.

Before this change output is:
![image](https://user-images.githubusercontent.com/16733618/69905823-5b4af680-13b1-11ea-9899-dfdd003411d3.png)

Output now looks better - but I need to fix the return code 
![image](https://user-images.githubusercontent.com/16733618/69906210-d44d4c80-13b7-11ea-8169-888b40796722.png)

Related failures: https://github.com/pandas-dev/pandas/issues/30031
CI finally passed. @datapythonista @WillAyd to merge 
Thanks @WillAyd
CI: Adding GitHub action to assign issues based on comment
State: closed

Currently, only maintainers and triagers can assign issues.

This implements a hack, so when any contributor writes the keyword `take` in an issue, GitHub actions will automatically assign the issue to the commenter.

I think this should be very helpful once people start to assign issues to themselves, since adding the `Assigned to nobody` filter should let people find issues that nobody is working on. May be in the future we want to implement another action to unassign issues after a period of inactivity, but so far this should be much better than just having people adding comments.

@pandas-dev/pandas-core what do you think?


Labels:
name: CI
description: Continuous Integration

Comments:
Sounds good. Not a huge blocker for me just a counter point, but certainly open to it. Sorry if missed discussion on it during dev chat

Sent from my iPhone

> On Nov 16, 2019, at 5:59 PM, Marc Garcia <notifications@github.com> wrote:
> 
> ﻿
> @datapythonista commented on this pull request.
> 
> In doc/source/development/contributing.rst:
> 
> > @@ -24,6 +24,27 @@ and `good first issue
>  where you could start out. Once you've found an interesting issue, you can
>  return here to get your development environment setup.
>  
> +When you start working on an issue, it's a good idea to assign the issue to yourself,
> My view is that now people is already locking issues for themselves using a comment. And I assume most people don't start working on something if they see someone else said they are working on it.
> 
> For me that's reasonable. The problem that I see is that there is no way to get the list of issues nobody is working on. Using Assignees would make things easier:
> 
> No need to spend the time writing comments to say you want to work on something
> Creating a filter for issues not assign becomes trivial
> Unassigning yourself from an issue is trivial (and I guess more likely, since having your name formally in assignees should give certain sense of responsibility)
> It becomes trivial to know if someone is assign, without having to read possibly ambiguous comments (e.g. "is it ok if I work in this?")
> I guess implementing a bot that unassignes issues after some time (a week?) of inactivity should be easy
> I'm optimistic that if we start using assignees, we could possibly create 200 issues for small fixes (e.g. small changes to docstrings), label them, promote somewhere (social media, sprints...), and we should be able to provide a link where people knows what is pending (not assigned).
> 
> We'll see how this works in practice, but I think it's worth trying. I don't expect this discouraging people or making things more inefficient. But it can possibly make it easier for people who want to contribute, to find things to work on.
> 
> —
> You are receiving this because you are on a team that was mentioned.
> Reply to this email directly, view it on GitHub, or unsubscribe.

ok let's try this
CI: fix flake C413, C414, C416 errors
State: closed

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


Labels:
name: CI
description: Continuous Integration
name: Code Style
description: Code style, linting, code_checks

Comments:
Thanks @jschendel comments addressed
we should merge this, i.e. not wait for the postgres thing to get sorted out
sure lgtm 

go for it
REF: remove unnecessary _try_cast calls
State: closed

Orthogonal to #29641.

_try_cast is a no-op if passed a scalar, so is unnecessary for some reduction/aggregation ops.  After #29641 i think we can get rid of more of these calls.

Labels:
name: Refactor
description: Internal refactoring of code
name: Groupby

Comments:
great! love code removal!
REF: make _aggregate_series_pure_python extraction behave like the cython version
State: closed

After some investigation it turns out that the DTA casting in `_try_cast` is made necessary because we incorrectly pass datetime64tz to `_aggregate_series_fast`, which calls the cython `libreduction.SeriesGrouper`, which casts the input to `ndarray`, losing the timezone.  By not going through the cython path for datetime64tz, we avoid the need to re-cast.

That in turn surfaces a new problem, which is that _aggregate_series_pure_python checks the first group's result slightly differently than the cython version does (see libreduction._extract_result).  This changes the way the pure-python version does it to more closely match the cython version. I intend to make these match more precisely in an upcoming pass.

If merged, makes #29589 unnecessary.

cc @jreback @jorisvandenbossche @WillAyd 

Labels:
name: Refactor
description: Internal refactoring of code
name: Groupby

Comments:
Hello @jbrockmendel! 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-17 16:02:37 UTC
Thanks for the explanation! 
What is the impact on performance of not using the cython path for datetimetz ? 

Can you add the examples I gave in the other PR as test cases?

> Can you add the examples I gave in the other PR as test cases?

Good idea, will do.

>  What is the impact on performance of not using the cython path for datetimetz ?

No idea.
thanks.

are these incorrect on 0.25.3? if so, need a release note, pls add in a followup.
> are these incorrect on 0.25.3? if so, need a release note, pls add in a followup

will do
> > What is the impact on performance of not using the cython path for datetimetz ?
>
> No idea.

Could you then check it?
Refactor string methods for StringArray + return IntegerArray for numeric results
State: closed

Intended as an alternative to https://github.com/pandas-dev/pandas/pull/29637.

This is a much smaller change. It only changes the codepath for StringDtype.
I think this is OK since someday (well down the road) we'll want to deprecate
the `.str` accessor on object-dtype Series. When we enforce that, we can just
delete the entire old implementation.

The API change is limited to always returning Int64Dtype for numeric outputs, rather than int if there's no NAs and float if there are any.
When BoolArray is done, we'll change that for the boolean-returning ones.

As a side benefit, we get a nice perf boost, since we have deterministic output dtypes we
can skip an object-dtype allocation.

```python
In [2]: s = pd.Series(['a'] * 100_000 + [None], dtype="string")

In [3]: t = s.astype(object)

In [4]: %timeit s.str.count("a")
50.7 ms ± 2.21 ms per loop (mean ± std. dev. of 7 runs, 10 loops each)


In [5]: %timeit t.str.count("a")
62.7 ms ± 742 µs per loop (mean ± std. dev. of 7 runs, 10 loops each)
```

Labels:
name: Performance
description: Memory or execution speed performance
name: Strings
description: String extension data type and string data

Comments:
> As a side benefit, we get a nice perf boost

Maybe a copy paste error, but the example doesn't show much performance boost :) (less than a percent)
This looks good to me! I suppose this is simpler than your other PR
Is this time-sensitive or can i wait and look at it on Monday?
No rush on this.

I did fail on the copy paste. The speed up is about 15% IIRC. 
Just the clipboard failure: https://github.com/pandas-dev/pandas/issues/29676
CI failure is from https://github.com/pandas-dev/pandas/issues/29514
Thanks for the review. Addressed all the comments I think.

After this, I'll have a small change at https://github.com/pandas-dev/pandas/pull/29597 to get it using the new methods (there's some no unnecessary changes in https://github.com/pandas-dev/pandas/pull/29597 that need to be removed).
Just the clipboard CI failure again.
@jreback thoughts on the conversation around the signatures? I think this is slightly blocking the pd.NA PR now. 

> On Nov 19, 2019, at 06:01, Joris Van den Bossche <notifications@github.com> wrote:
> 
> ﻿
> @jorisvandenbossche approved this pull request.
> 
> —
> You are receiving this because you authored the thread.
> Reply to this email directly, view it on GitHub, or unsubscribe.

Merged master, and writing up an issue for the followup.
https://github.com/pandas-dev/pandas/issues/29710 for followup.
Thanks all!
Convert bins to numeric for histogram if a string
State: closed

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


Labels:
name: Visualization
description: plotting

Comments:
Yes I'll work on creating a test.
can you merge master and add a test
@wesbarnett does this type of bins handling come from other packages? We only have this documented as working for `integer or sequence`
@WillAyd it comes from matplotlib where you can pass things like "auto". See #29484.
Makes sense. Can you update the docstring as well then? 
Hello @wesbarnett! 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-10-03 13:13:30 UTC
@WillAyd I only see the docstring for hist indicating an integer is supported. I've added a blurb taken from the matplotlib docstring to indicate that a string is supported.

I actually misspoke in my last comment - this is provided by numpy's histogram method, which matplotlib uses.

Is sequence also supported?
@wesbarnett can you take a look at last round of comments?
@wesbarnett this also needs a whatnew note (bug fixes plotting in 1.1), pls ping on green when ready.
> @wesbarnett can you take a look at last round of comments?

@WillAyd See the previous thread. I think I addressed everything other than whatsnew entry. Sorry if I missed anything.
@wesbarnett can you merge master
Merged master to fix the upstream CI issue.

@wesbarnett do we have a test for this that uses `.plot.hist()`?
@TomAugspurger No test for `.plot.hist()` in this. I can add some.
@jreback Okay, should be good now. I merged master again.
@wesbarnett merging master should fix the CI failures.
@wesbarnett is this still active? Can you move note to 1.2?
@WillAyd @jreback this is ready for review. One of the builds failed to run. Not sure how to retrigger that.
ping @WillAyd @jreback 

@wesbarnett can you merge master again?
closing as stale. this looks like an ok soln, though likely needs some more tests.
REF/API: String methods refactor
State: closed

edit: probably closing this: See https://github.com/pandas-dev/pandas/pull/29640 for a simpler alternative.

In preparation for https://github.com/pandas-dev/pandas/pull/29597, this changes the string methods implemetation to

* Use masks to apply the function just to valid values.
* Disables all unnecessary inference on the result.

This works quite well for the new StringDtype, since we know the values are always string. The object-dtype behavior is a bit more complex. I'll note that inline below.

Additionally, it fixes https://github.com/pandas-dev/pandas/issues/29624

edit: Oh, forgot another change. String methods returning numeric values will return a nullable integer type. So `Series[string].str.count('a')` returns an Int64Dtype, rather than a maybe int or maybe float (depending on the presence of NAs). Will document that.

Labels:
name: Refactor
description: Internal refactoring of code
name: Strings
description: String extension data type and string data

Comments:
cc @jorisvandenbossche 
> long term, we might want something like lib.map_infer but where we can pass a mask? (so we don't need to filter first, and assign to a subset)

there *already* is a lib.map_infer_mask FYI
> there already is a lib.map_infer_mask FYI

Which is what was being used before actually. Tom, what's the reason for no longer using it? Being able to pass the 'na' value might be useful (now it takes the original value from the array, which is not always what you want I suppose)
> did you check performance? More or less the same as before?

checking now. Slowdown in things like `str.get` and `.str.decode` in a few cases.

Will look into re-using `map_infer_mask`. If I can pass through an `na_result` rather than the original, that may be enough.
I wonder if it makes sense to just apply this change to just StringDtype? That'd keep the diff a lot smaller at least, and is probably less like to introduce new bugs.
Closing in favor of https://github.com/pandas-dev/pandas/pull/29640
TST: add test case for user-defined function taking correct path in groupby transform
State: closed

I kept this one in place because I thought fast_path could be user-defined (see comment on L1425), but it turns out we define it in _define_paths a few lines above.  So this try/except is unnecessary.

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

Comments:
> it turns out we define it in _define_paths a few lines above

But the `func` in `_define_paths` can still be a UDF?
@jbrockmendel these are all failing for the web / docs stuff?
> But the func in _define_paths can still be a UDF?

Yes, but the sole call to _choose_path is itself wrapped in a `try`/`except TypeError`.

It's entirely possible that I'm missing something.  In recent threads you've shown a talent for finding corner cases that are currently missing from the tests.  Any ideas for what cases will raise here that we should worry about?
> these are all failing for the web / docs stuff?

Not sure yet whats going on there.  Could well be an example we can/should add to the tests!
> Not sure yet whats going on there. Could well be an example we can/should add to the tests!

Probably yes, it's in any case a transform with a UDF that is failing
@jorisvandenbossche i found the case from cookbook.rst that is failing in this PR, but it isn't clear to me that it is behaving correctly on master either.  I'd like to get a second opinion.

```
df = pd.DataFrame({'A': [1, 1, 2, 2], 'B': [1, -1, 1, 2]})
gb = df.groupby('A')

def replace(g):
    mask = g < 0
    return g.where(mask, g[~mask].mean())

result = gb.transform(replace)

>>> result
     B
0  1.0
1 -1.0
2  1.5
3  1.5
```

I expected `result.loc[1]` to match `result.loc[0]`.  Can you double-check me on this?

It's what the function seems to be doing:

```
In [22]: group1 = gb.get_group(1)

In [23]: replace(group1['B']) 
Out[23]: 
0    1
1   -1
Name: B, dtype: int64
```

So maybe the function is doing something unexpected, but it seems the `transform` is correctly using the result of the function on the group.

I find `where` always a difficult one to reason about, but basically this `where` call is keeping the original negative values, and replacing the positive values with the mean of those positive values. So in this case, that result seems correct to me (as the mean of the positive values is also 1).
@jorisvandenbossche for the immediate purpose of this PR, I'm implementing a test case where fast_path raises, but I don't want to use a not-clearly-correct case.  Can you suggest one?  (this might be obvious, but i need caffeine)

The follow-on issues are a) making sure the example in the docs is correct and b) making sure `where` is correct.
As far as I understand, the example in the docs *is* correct
Updated.  Now the behavior is unchanged, just added a test that requires the current behavior.  Thanks for helping me understanding it @jorisvandenbossche 
thanks @jbrockmendel