BrandonFors/pandas-github-issues huggingface dataset:
Fix #28552 Add test for DataFrame min/max preserve timezone
State: closed

- [x] closes #28552
- [x] tests added / passed
- [x] passes `black pandas`
- [x] passes `git diff upstream/master -u -- "*.py" | flake8 --diff`
- [ ] whatsnew entry _(not applicable - no functionality added)_

Here's a test discussed in #28552. `mean` and other aggregates except for `min` and `max` are not covered since they don't support datetimes - it's a nan being returned.


Labels:
name: Testing
description: pandas testing functions or related to the test suite
name: Reshaping
description: Concat, Merge/Join, Stack/Unstack, Explode
name: Timezones
description: Timezone data dtype

Comments:
Hello @dair-targ! 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-05-12 11:57:56 UTC
@mroeschke thanks for review! I hope I've addressed all the issues - would you mind to review it again?
thanks @dair-targ
BUG: DTI/TDI intersection result names
State: closed

- [ ] closes #xxxx
- [ ] tests added / passed
- [ ] passes `black pandas`
- [ ] passes `git diff upstream/master -u -- "*.py" | flake8 --diff`
- [ ] whatsnew entry


Also improve _can_fast_intersect to catch the missing case where we can fast-intersect.  Identified that _most_ of the remaining places where we do freq inference are not tested, so may be removeable/deprecateable

Labels:
name: Frequency
description: DateOffsets

Comments:
> does this need a whatsnew?

yes, just pushed with a note added

>  Identified that most of the remaining places where we do freq inference are not tested, so may be removeable/deprecateable

on board with this for follow-up?
DEPR: Deprecate try_sort
State: closed

xref #33336 

While working on another PR, and @WillAyd brought up `try_sort`, and after a quick look, I don't think `try_sort` is needed in that check method in `indexes/api.py`. (I also dont think it is right to have try_sort there), so deprecate it.



Labels:
name: Clean

Comments:
yep this is fine, ping on green.
this one needs to wait until #33336 gets merged, since `try_sort` is still being used on master. @jreback 

I will leave it as WIP for now and then rebase once okay.

Can you merge master on this?
ping @WillAyd @jreback 
thanks @charlesdong1991
BUG: recognize M8[ns, tzstr]
State: closed

- [ ] closes #xxxx
- [x] tests added / passed
- [x] passes `black pandas`
- [x] passes `git diff upstream/master -u -- "*.py" | flake8 --diff`
- [ ] whatsnew entry

ATM `dtype == "M8[ns, UTC]"` fails while `dtype == "datetime64[ns, UTC]"` succeeds

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

Comments:
Is this needed to support? (what's the use case?) 
M8 is a numpy short-hand name, but tz-aware dtype is not a numpy dtype.
lgtm. 
> see comment

I find this ok, all we are asserting here is that `datetime64==M8` which is true
> Is this needed to support? (what's the use case?)

needed?  No.  convenient?  yes.

Until recently there as a test that checked `dtype == "M8[ns, US/Eastern]"` that would have failed if it wasn't missing the "assert".
What is the convenience? Just less typing?


> What is the convenience? Just less typing?

Sure.  Or maybe "lease surprise" is more accurate.  e.g. `is_dtype_equal(dtpye, "M8[ns, US/Eastern]")` is True ATM while `dtype == "M8[ns, US/Eastern]"` is False
OK, but then it is fixing a bug, not providing something for convenience. 

Eg, it seems that `pd.Series([1, 2, 3]).astype("M8[ns, UTC]")` already works. Then of course `.dtype == "M8[ns, UTC]"` should also work. 
Do you know if it is tested that `astype` / `pandas_dtype(..)` works with this notation? If not, can you add a test for that?

> If not, can you add a test for that?

added
BUG: Preserve Series/DataFrame subclasses through groupby operations
State: closed

This pull request fixes a bug that caused subclassed Series/DataFrames to revert to `pd.Series` and `pd.DataFrame` after `groupby()` operations. This is a follow-up on a previously abandoned pull request (#28573)

- [x] closes #28330
- [x] tests added / passed
- [x] passes black pandas
- [x] passes git diff upstream/master -u -- "*.py" | flake8 --diff
- [x] whatsnew entry

Labels:
name: Groupby

Comments:
I updated my branch with additional tests, and a revised entry in the whatsnew rst file. 

Please let me know if there are any additional recommendations for how to proceed with this PR.
Just to follow-up -- I have a few questions / comments on how best to proceed:

1. The change I made to appease mypy seems to pass all tests, but please let me know if you have a better way to handle that line.
2. Although I initially agreed with having `GroupBy.ngroup()` and `GroupBy.cumcount()` return `Series`, I think it makes sense to have them preserve subclasses (as I initially had it). This would make it so that all `groupby.agg()` / `groupby.transform()` operations return the calling subclass, rather than having two operations that return a different type. 

Thanks for your help,
Jack
Thanks for the mypy advice -- I managed to refactor that section a bit to only call `cast()` if needed and avoid calling the constructor again. 

The one additional change made here is to make sure `GroupBy.size()`, `GroupBy.ngroup()`, and `GroupBy.cumcount()` preserve subclassed types. While you had previously mentioned just keeping these return types as `Series`, I think these changes minimize the possibility for surprise when subclassing `DataFrame`/`Series` by ensuring that groupby reductions/transformations preserve the subclasses.
@jreback -- please let me know if there are any additional changes to this PR that you think are necessary. 
@jreback I made the recommend changes:
- I reverted the changes in `DataFrameGroupBy._cython_agg_blocks()` because they did not prove necessary to pass my tests.
- I added a `GroupBy._constructor` property to simplify the calls in `ngroup`, `cumcount`, and `size`.
@simonjayhawkins Thanks for the comment -- I agree that line should have used `self.obj._constructor`, and changed it 
As per the discussion above, the GroupBy property has been renamed to `_obj_1d_constructor` to clarify its expected return type, and I refactored it to avoid using an `else` statement. 
thanks @JBGreisman very nice, thanks for sticking with it and being responsive!
no worries -- thanks for the help/suggestions!
BUG: TimedeltaArray+Period, PeriodArray-TimedeltaArray
State: closed

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

Labels:
name: Timedelta
description: Timedelta data type
name: Period
description: Period data type

Comments:
Can you explain what this PR is changing?
can you also add an example in the docs (maybe in the timedelta section)
> Can you explain what this PR is changing?

PeriodIndex - TimedeltaIndex always raises in master, but for some `freq` combinations should not.  This fixes that.
thanks
Fix Dataframe.apply documentation to include note regarding calling f…
State: closed

…unction twice on first element

- [ ] closes #33879
- [ ] tests added / passed
- [ ] passes `black pandas`
- [ ] passes `git diff upstream/master -u -- "*.py" | flake8 --diff`
- [ ] whatsnew entry


Labels:

Comments:
So looks like there is confusion over the fact that DataFrameGroupBy.apply only calls the function once but DataFrame.apply still calls twice right? Any interest in just trying to fix the latter?
Very interested, however i will need some guidance as i am not familiar with the internals of the project.
The fix to this on the GroupBy side was #24748 which may be helpful for investigation. Otherwise take a look at the apply implementation in pandas.core.frame
DOC: Remove TODO from `test_openpyxl`
State: closed

xref #29862 

Brought up by @jbrockmendel seems the `xfail` has been removed and version has been unpinned, just a tiny PR to remove this `TODO` from tests. probably people who fixed it forgot to remove this.

cc @jbrockmendel 


Labels:
name: Clean

Comments:
LGTM, the CI failure is unrelated
Thanks @charlesdong1991
DOC: Remove deprecated Y and M from to_timedelta
State: closed

- [ ] closes #xxxx
- [ ] tests added / passed
- [x] passes `black pandas`
- [ ] passes `git diff upstream/master -u -- "*.py" | flake8 --diff`
- [ ] whatsnew entry

[`to_timedelta()`](https://github.com/pandas-dev/pandas/blob/master/pandas/core/tools/timedeltas.py#L84-L88) blocks those arguments anyway, so no reason to include them in the docstring.

Labels:
name: Docs

Comments:
thanks @taljaards
DOC: Fix groupby.agg/transform rst reference and numba references
State: closed

- [x] xref https://github.com/pandas-dev/pandas/pull/33388#issuecomment-618808443
- [x] tests added / passed
- [x] passes `black pandas`
- [x] passes `git diff upstream/master -u -- "*.py" | flake8 --diff`


Labels:
name: Docs
name: Groupby

Comments:
looks fine the CI/checks buidl is failing
CLN: address FIXME/TODO/XXX comments
State: closed

- changed XXX comments to TODO (much more grep-friendly, since we sometimes use XXX for a `name` attribute.
- Removed some now-fixed FIXME/TODO comments
  - tests.arithmetic.test_datetime64, tests.arithmetic.test_period, test_conversion, tests.indexes.common, test_date_range, test_css, scalar.timedelta.test_arithmetic
- Added some TODO(EA2D) comments
- Removed some `TODO(wesm): unused?` comments

Labels:
name: Clean

Comments:
thanks
ENH: Add na_value argument to DataFrame.to_numpy
State: closed

- [x] closes #33820
- [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: Missing-data
description: np.nan, pd.NaT, pd.NA, dropna, isnull, interpolate
name: ExtensionArray
description: Extending pandas with custom dtypes or arrays.

Comments:
Another interesting issue is the behavior for mixed numeric and non-numeric data types. In the docstring we say that these will promote to object, which does happen but apparently numpy decides to convert the datetime to int beforehand anyways (we get the expected output if we explicitly ask for object):
```python
[ins] In [1]: df = pd.DataFrame({"A": [1, 2], "B": [3.0, 4.5]})                                                                                                                                              

[ins] In [2]: df['C'] = pd.date_range('2000', periods=2)                                                                                                                                                     

[ins] In [3]: df.to_numpy()                                                                                                                                                                                  
Out[3]: 
array([[1, 3.0, 946684800000000000],
       [2, 4.5, 946771200000000000]], dtype=object)

[ins] In [4]: df.to_numpy(dtype=object)                                                                                                                                                                      
Out[4]: 
array([[1, 3.0, Timestamp('2000-01-01 00:00:00')],
       [2, 4.5, Timestamp('2000-01-02 00:00:00')]], dtype=object)
```
So I think you need to deal with this deeper into the internals, as I mentioned before. It is there that the custom pandas logic to decide on the resulting dtype is done (which is indeed different from numpy's result dtype)
Check BlockManager.as_array -> this also already handles the case of a single Block (to keep the no copy behaviour there).

I think we can expand that method to support na_value
thanks @dsaxton
DOC: Fix groupby.agg/transform API reference and add numba arguments to documentation
State: closed

- xref https://github.com/pandas-dev/pandas/pull/33388#issuecomment-618808443
- [x] passes `black pandas`
- [x] passes `git diff upstream/master -u -- "*.py" | flake8 --diff`

<img width="927" alt="Screen Shot 2020-04-27 at 11 19 16 PM" src="https://user-images.githubusercontent.com/10647082/80453833-e20eeb80-88dd-11ea-9acc-ef322cb59df5.png">
<img width="934" alt="Screen Shot 2020-04-27 at 11 19 33 PM" src="https://user-images.githubusercontent.com/10647082/80453838-e4714580-88dd-11ea-85b2-735d327ee686.png">
<img width="922" alt="Screen Shot 2020-04-27 at 11 19 53 PM" src="https://user-images.githubusercontent.com/10647082/80453844-e6d39f80-88dd-11ea-8c49-a9d35fd80e0c.png">
<img width="927" alt="Screen Shot 2020-04-27 at 11 20 16 PM" src="https://user-images.githubusercontent.com/10647082/80453851-e935f980-88dd-11ea-8c62-b92043ed26e4.png">


Labels:
name: Docs

Comments:
Hello @mroeschke! 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-04-28 17:18:40 UTC
BUG: preserve freq in DTI/TDI factorize
State: closed

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

Labels:
name: Frequency
description: DateOffsets
name: ExtensionArray
description: Extending pandas with custom dtypes or arrays.
name: Mothballed
description: Temporarily-closed PR the author plans to return to

Comments:
@mroeschke do we need to pin a numba dep?
The test failures don't look numba related, but I may need to silence more warnings in some tests 

EDIT: I have this `filterwarning` on all numba tests that use parallel. Locally it's filtering the warning not sure why it's raising here

`@pytest.mark.filterwarnings("ignore:\\nThe keyword argument")`
hmm ATM this fixes dti.factorize() but not pd.factorize(dti)

<b>update</b> fixed
> can you explain why we are exposing .factorize() as a public method on Index?

No idea, its there on both Index and Series in the status quo.
Gentle ping, this is a blocker for freq-check in assert-index-equal
you haven’t answered my question above

> you haven’t answered my question above

you asked why we are exposing Index.factorize, and I answered [here 
](https://github.com/pandas-dev/pandas/pull/33836#issuecomment-621896133) that i am not aware of a good reason, but we do it in master.
Mothballing.  If no one else cares about these bugs, not worth pursuing.
REF/CLN: Parametrize _isna
State: closed

Replaces two nearly identical isna functions with one by parametrizing over the difference

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

Comments:
LGTM.

_isna_ndarraylike should get a similar treatment; IIRC there is an issue with EAs not handling `old` correctly
Thanks!
DEPR: Timestamp.freq
State: closed

- [x] closes #15146
- [x] tests added / passed
- [x] passes `black pandas`
- [x] passes `git diff upstream/master -u -- "*.py" | flake8 --diff`
- [x] whatsnew entry

The motivation here is that I've concluded that DatetimeArray.freq _must_ be removed, so for internal consistency Timestamp.freq should be removed too.

The main user-facing behavior this will affect is Timestamp.is_month_start, is_month_end, is_quarter_start, is_quarter_end, is_year_start, is_year_end, for which I guess we'll offer users an alternative(?)

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

Comments:
> The main user-facing behavior this will affect is Timestamp.is_month_start, is_month_end, is_quarter_start, is_quarter_end, is_year_start, is_year_end, for which I guess we'll offer users an alternative(?)

I think all of these can be defined without frequency. Looks like there's already "no frequency" code path for these

```
        if self.freq is None:
            # fast-path for non-business frequencies
            return self.day == 1 and self.month % 3 == 1
```
can you link to the deprecation removal issue as well
> I think all of these can be defined without frequency. Looks like there's already "no frequency" code path for these

You're right.  The potential issue is if users want to get the old (i.e. current) behavior that as if there were a freq.  I guess we can tell them to do `DatetimeIndex([ts], freq=freq).whatever`
The current PR seems to generate a lot of warnings (from looking at the test changes) that shouldn't be propagated to the user? 
(eg boxing a DatetimeIndex, converting to object dtyp, ... And I also noticed that accessing a single element from a DatetimeIndex raises the warning, and the DatetimeIndex repr, ...) 

> The main user-facing behavior this will affect is Timestamp.is_month_start, is_month_end, is_quarter_start, is_quarter_end, is_year_start, is_year_end, for which I guess we'll offer users an alternative(?)

Can you show with an example what is actually changing here?
closing to clear the queue
TST: ensure groupby get by index value #33439
State: closed

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


Labels:
name: Testing
description: pandas testing functions or related to the test suite
name: Groupby
name: ExtensionArray
description: Extending pandas with custom dtypes or arrays.

Comments:
Hello @sam-cohan! 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-04-29 22:38:27 UTC
Failures seems unrelated to my change.
/azp run

<samp>
Azure Pipelines successfully started running 1 pipeline(s).<br>

</samp>
thanks @sam-cohan
BUG: DataFrame[object] + Series[dt64], test parametrization
State: closed

- [ ] closes #xxxx
- [x] tests added / passed
- [x] passes `black pandas`
- [x] passes `git diff upstream/master -u -- "*.py" | flake8 --diff`
- [ ] whatsnew entry

In the course of parametrizing tests, found a failing case.  The edit in ops.dispatch fixes it.

Labels:
name: Bug
name: Testing
description: pandas testing functions or related to the test suite
name: Dtype Conversions
description: Unexpected or buggy dtype conversions

Comments:
thanks, in theory could add a whtasnew note for the (obscure) case
TST: Separate expanding_apply and rolling_apply out of consistency
State: closed

xref #30577 #30486 

This is the second step for breaking up giant consistency test, this PR separates consistency tests about `expanding_apply` and `rolling_apply` out from their big `expanding_consistency` and `rolling_consistency` tests.

The followup after this would be removing `self.data` initiation from `init` and convert to either fixture or funcs with yields so as to gradually move tests out of the whole consistency class and easier for parametrization instead of current looping.

cc @jreback 


Labels:
name: Testing
description: pandas testing functions or related to the test suite
name: Window
description: rolling, ewma, expanding

Comments:
great thanks.

yeah parameterization would be great to remove some of the inner loops.
ERR: Add NumbaUtilError
State: closed

- xref https://github.com/pandas-dev/pandas/pull/33388#discussion_r415172278
- [x] tests added / passed
- [x] passes `black pandas`
- [x] passes `git diff upstream/master -u -- "*.py" | flake8 --diff`

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

Comments:
LGTM
thanks @mroeschke
TST: Clean moments consistency
State: closed

As discussed in https://github.com/pandas-dev/pandas/pull/30577#issuecomment-570014613

This PR is the first step to achieve it, it basically does one thing: break up the giant consistency test in `common` into four tests based on test contents: `var`, `std`, `cov`, and `series data`.

cc @jreback 

Labels:
name: Testing
description: pandas testing functions or related to the test suite
name: Window
description: rolling, ewma, expanding

Comments:
great looks good. when doing these types of reactoring just make sure that we don't lose tests (a bit tricky to check here as you are actually changing the number of tests), but lgtm.
thanks @charlesdong1991 
thanks @jreback i will keep followups smaller, and easier to check if any test is lost!
BUG: Don't raise in DataFrame.corr with pd.NA
State: closed

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


Labels:
name: Numeric Operations
description: Arithmetic, Comparison, and Logical operations
name: NA - MaskedArrays
description: Related to pd.NA and nullable extension arrays

Comments:
> can you also use `.convert_types()` (e.g. like the OP) in the tests

Hmm, I think we wouldn't want that in the test because the functionality we're testing is corr / cov rather than convert_dtypes (the original bug report wasn't minimal, convert_dtypes was working okay)?
thanks @dsaxton 
when implement ``DataFrame.to_numpy(na_value=np.nan)`` let's update this (if you can note that in the issue).
TST: Added message to bare pytest.raises in test_join_multi_levels (#…
State: closed

…30999)

Updated tests in test_join_multi_levels to include messages for pytest.raises.

- [ ] closes #xxxx
- [x] tests added / passed
- [x] passes `black pandas`
- [x] passes `git diff upstream/master -u -- "*.py" | flake8 --diff`
- [x] whatsnew entry


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

Comments:
thanks @DominicG88 
Great, thanks @jreback 👍
BUG: can't concatenate DataFrame with Series with duplicate keys
State: closed

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


Labels:
name: Indexing
description: Related to indexing on series/frames, not to indexes themselves
name: Reshaping
description: Concat, Merge/Join, Stack/Unstack, Explode

Comments:
lgtm. let's merge on green.
Thanks - sure, it's green now
Thanks @MarcoGorelli