BrandonFors/pandas-github-issues huggingface dataset:
Parametrize JSON tests
State: closed

lot more to be done here but split these up as some of the large functions are difficult to highlight failures while working on extension module. Improved coverage and identified some inconsistencies in process (marked with TODOs) which can be tackled as follow ups

Labels:
name: Testing
description: pandas testing functions or related to the test suite
name: IO JSON
description: read_json, to_json, json_normalize

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 2019-09-17 04:19:20 UTC
@WillAyd can you rebase
looks like PY35 is not defined?
My bad - good find
Finally green...the parametrization process revealed quite a few inconsistencies with our JSON parsing, namely:

 - read_json sorts when `orient="index"` and `numpy=False` (default value)
 - read_json sorts when Py35 and, `numpy=False` (default value) and `orient="columns"`
 - read_json returns `np.int32` on Windows and 32 bit platforms when `orient` is anything other than `split`
 - read_json with `orient="split"` does not preserve numeric column / index values, though it should be able to
 - read_json with `dtype=False` will read in `null` values as `None` instead of `np.nan`
 

The Py35 issues are probably moot at this point but happy to open up follow up issues for the rest if no other changes are required here
nice, ill give this a read
Anyone have thoughts on this one? I think should be good to go
Looking commit-by-commit most of them look pretty straightforward, but the _check_all_orients refactor is pretty imposing.  Would doing this in smaller pieces be burdensome?

Are the tests substantively changed or just parametrizing existing tests?

Sorry for the delay in review.
@WillAyd gonna make another pitch for splitting this into smaller pieces to keep forward momentum
Sure no problem. Are you just looking for `test_frame_from_json_to_json` to be in a separate PR?
> Are you just looking for test_frame_from_json_to_json to be in a separate PR?

I think that'd go a long way, yah.
Sorry for delay but should be done now. I ultimately plan to just revert these last two commits in a subsequent PR but I think this should make review more manageable on this one. lmk what you think
I think I've marked all the funcs with clear one-to-one changes.  Happy to merge those, the others I need more time and caffeine
@WillAyd can you rebase and i'll make a big push to get this reviewed this afternoon
Done and thanks! These parametrization ones aren't the most fun to review so much appreciated
@WillAyd a handful of comments, generally LGTM.  This is going to be much nicer when you're done here, thanks for doing it.
OK last 3 commits should address outstanding comments. Let me know what you think and thanks again for the review
@WillAyd LGTM, thanks.

There are a couple of threads in the diff that are unresolved that would make for nice follow-ups, but nothing that is a blocker.  Merging.
Update sample() documentation
State: closed

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


Labels:
name: Docs

Comments:
Yeah, this is clearer.
Thanks, @lizsaret
Add example for Series.str.slice()
State: closed

Add example to show that negative values for start are allowed for Series.str.slice().

- [ X] closes #27802
- [ ] tests added / passed
- [ ] passes `black pandas`
- [ ] passes `git diff upstream/master -u -- "*.py" | flake8 --diff`
- [ ] whatsnew entry


Labels:
name: Docs

Comments:
Hello @avelineg! 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-08-09 03:34:36 UTC
Thanks, @avelineg
BUG: Fixed groupby quantile for listlike q
State: closed

Closes #27526

Just a WIP right now. This is under tested for 

- [ ] `groupby(sort=False)`
- [ ] non-sorted `q`
- [ ] `groupby(multiple levels)`

Labels:
name: Groupby

Comments:
@WillAyd @jbrockmendel do you have time to take a look at this?
will look in AM
@jbrockmendel I added your alternative implementation as a TODO once the MultiIndex issue (https://github.com/pandas-dev/pandas/issues/10710) is fixed.

> Darn. Can you add a my-idea-doesnt-work case to the tests

It's covered.
CI failure is just codecov.
Merging in 1 hour if there aren't any objections (only CI failure is codecov).
BUG: Fix groupby quantile segfault
State: closed

Validate that q is between 0 and 1.

Closes #27470

Labels:
name: Groupby
name: Segfault
description: Non-Recoverable Error

Comments:
LGTM
Thanks @TomAugspurger 
Going through the 0.25.3 release the test for this showed up in the diff while cherry-picking. On a second review I don't think this was fully back ported (?). I see the whatsnew note but not the test or the changes to groupby.pyx

Unless I am overlooking something should I pick this up for the 0.25.3 release as well?
COMPAT: restore shape for 'invalid' Index with nd array
State: closed

closes #27775


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

Comments:
>  Would it help if we put an (invisible by default) DeprecationWarning in here?

What would you deprecate? It's creating the 2D Index that we should deprecate, not necessarily accessing its shape. 
But deprecating that should go on master, not for 0.25.1 I think. 

To be clear, I only see this as a fix for 0.25.1, and we should further discuss how we want to fix this long term on master (convert to ndarray, or deprecate/raise error)
> What would you deprecate?

Not really a deprecation. Just a "hey this is wrong" warning that's invisible by default in `Index.shape`. I just don't know how matplotlib is going to catch this otherwise (though maybe they have a central place where they do their conversion to NumPy).
To be clear, do you propose that instead of this PR? (or as something that should be added to this PR?)

(for me that is again part of a separate longer term solution discussion)
seems ok to me for 0.25.1, I think we should raise starting in 1.0.0 for this on consruction; its invalid.
> To be clear, do you propose that instead of this PR? (or as something that should be added to this PR?)

Not instead, in addition to. Let's see how the index construction goes.
@TomAugspurger Yes: https://github.com/matplotlib/matplotlib/pull/15007 although you may get a fix out before we do though :woman_shrugging: .
@jreback What type would you raise?  If it is `TypeError` or `IndexError` it will "just work" with Matplotilb.
Turns out this used to be extra goofy:

```python

In [13]: import pandas

In [14]: pandas.__version__
Out[14]: '0.24.2'

In [15]: import pandas as pd

In [16]: df = pd.DataFrame([1, 2])

In [17]: df.index[:, None].shape
Out[17]: (2, 1)

In [18]: df.index[:, None].ndim
Out[18]: 1

```
@tacaswell yes, it was buggy, it is still buggy, we just changed the way it was buggy (and this PR restores the previous way of buggy)

But let's keep all that discussion about additional fixes, returning array or erroring, .. for a proper issue about it (I will open one).
Opened an issue here: https://github.com/pandas-dev/pandas/issues/27837
REF: implement should_extension_dispatch
State: closed

This uses should_extension_dispatch in _arith_method_SERIES.  We'll be able to use it in _comp_method_SERIES after #27803. (this is orthogonal to 27803 so doesn't matter what order they go in)

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

Comments:
Updated and green
@jreback gentle ping; this is a step toward the Plan B discussed on the call for arithmetic perf
thanks
BUG: Series.rename raises error on values accepted by Series construc…
State: closed

- closes #27813

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

Comments:
Needs a test
Hello @lgmsantos! 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-08-22 11:33:56 UTC
How can I fix those failed checks? They seem unrelated to the changes.
Try merging master and re-pushing?
Merged master. Ping on green.
CI: remove pytest pins
State: closed


Labels:
name: CI
description: Continuous Integration

Comments:
do we need to note the pytest bump in 0.25.1 docs? (not a big deal either way). might need to make the same change on pandas-wheels
No I don't think so. It's only a cross compatibility issue between pytest &
pytest-xdist.

On Thu, Aug 8, 2019 at 7:14 AM Jeff Reback <notifications@github.com> wrote:

> do we need to note the pytest bump in 0.25.1 docs? (not a big deal either
> way). might need to make the same change on pandas-wheels
>
> —
> You are receiving this because you modified the open/close state.
> Reply to this email directly, view it on GitHub
> <https://github.com/pandas-dev/pandas/pull/27809?email_source=notifications&email_token=AAKAOIT2WIUN723N3SAZH3TQDQE2VA5CNFSM4IKELUOKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD33NKFY#issuecomment-519492887>,
> or mute the thread
> <https://github.com/notifications/unsubscribe-auth/AAKAOIQVZDRMEVTPQN2UPY3QDQE2VANCNFSM4IKELUOA>
> .
>
[WIP] Improve CI/CD pipeline performance
State: closed

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


Labels:
name: CI
description: Continuous Integration

Comments:
Out of curiosity, how big is the perf difference? 
> Out of curiosity, how big is the perf difference?

It is WIP. WIll know once I make some progress.
does this change perf? can you merge master as well
@WillAyd @jreback yes, it is not reflecting on the performance improvement. I was off a couple of weeks now I'm back on. Will check it
@bhavaniravi if you want to continue on this, pls merge master and post a comparison of before / after.
I think this is stale or not really giving the results we are after, so closing. @bhavaniravi if you disagree ping and can reopen for sure

Thanks as always!
REF: Simplify _comp_method_SERIES
State: closed

- Do raising checks earlier on, matching how this is done elsewhere (moving towards #23853) 
- Comment on line 1034 notes possibly-inconsistent behavior that we should either change or add an explanatory comment for.
- Ditto line 1058
- Before long we'll be able to collapse all of the dispatch_to_extension_op cases into one case; I'm still troubleshooting some corner cases of this on another branch.
- After this, `na_op` is only called in one place, meaning it will be easier to make further de-nesting/de-closuring simplifications in follow-ups.
- Putting all the finalize/_constructor business at the end will make it easier to separate out the array-specific middle and apply it to a) PandasArray and b) Block-wise.

Labels:
name: Clean

Comments:
thanks @jbrockmendel
DOC: add print statement in to_latex example
State: closed

changed `df.to_latex` to `print(df.to_latex)`

closes #27789



Labels:
name: Docs
name: IO LaTeX
description: to_latex

Comments:
@jorisvandenbossche Done!
@jorisvandenbossche I am not able to understand what is causing the test failure. Can you help me out
@sameshl that seems as an unrelated failure that you can ignore (I need to check if there is already an issue about this failure, it might be a flaky test)
@sameshl Thanks a lot!
DEPR: Remove previously deprecated IntervalIndex.from_intervals
State: closed

Didn't see any references to `IntervalIndex.from_intervals` in the docs. 

We deprecated this before `IntervalArray` was implemented, so no `from_intervals` to worry about there; `IntervalArray` file just has a "shared" docstring for `from_intervals` that isn't actually shared.

Labels:
name: Deprecate
description: Functionality to remove in pandas
name: Interval
description: Interval data type

Comments:
thanks @jschendel
CLN: remove nested error handling
State: closed

The removed code is not covered in the test suite, so this makes for a nice little simplification.  Moreover, since the condition above on 1369-1373 is an ugly one, de-nesting this function will give us a good shot and further simplifying this code.

That said, this could be considered an API change, since now the "errors" kwarg doesn't actually do anything.

Labels:
name: Clean

Comments:
Some stuff to unpack here with your PR description!

> The removed code is not covered in the test suite, so this makes for a nice little simplification.

When did that become justification for removing code? 🙂

> That said, this could be considered an API change, since now the "errors" kwarg doesn't actually do anything.

Hmm...I'm always a little on the fence when it comes to simplifications that have API change side-effects, not matter how small.  I guess it's a judgment call, but I do appreciate the simplification.
> When did that become justification for removing code? 🙂

Many chunks of code were previously intended for Panel.  Non-covered code isn't necessarily removable, but it makes for a good place to look for un-needed code.

>  I guess it's a judgment call, but I do appreciate the simplification.

Yah, the real upside is that this makes it much simpler to remove _try_coerce_args, which would complete an ongoing simplification goal.

thanks @jbrockmendel
CLN: Assorted Cleanups
State: closed

- make PTA/DTA strftime() return ndarray instead of Index
- fix a xfailed Series[Period] test
- avoid imports of Index subclass in arrays.categorical and arrays.datetimelike
- some typing

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

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-08-07 00:56:59 UTC
thanks @jbrockmendel
TST: Add tests for groupby categorical values with axis=1
State: closed

This is xref https://github.com/pandas-dev/pandas/pull/27700#issuecomment-517703401

- [x] closes #13420
- [x] tests added / passed
- [x] passes `black pandas`
- [x] passes `git diff upstream/master -u -- "*.py" | flake8 --diff`
- [ ] whatsnew entry


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

Comments:
lgtm. @WillAyd 
Thanks @charlesdong1991 !
Fix miniconda path issue #26962
State: closed

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


Labels:
name: CI
description: Continuous Integration
name: Clean

Comments:
Thanks @bhavaniravi, really great to have this fixed, much cleaner now
Thank you @datapythonista I learned a lot in the process. Looking to contribute more on the devops side of things
BUG: fix construction of NonConsolidatableBlock with inconsistent ndim
State: closed

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


Labels:
name: Bug
name: ExtensionArray
description: Extending pandas with custom dtypes or arrays.

Comments:
cc @jbrockmendel 
I tried once to require that ndim be passed and found that some of the downstream tests failed because e.g. pyarrow uses make_block directly
> I tried once to require that ndim be passed

That's indeed another option to pass `self.ndim` to all calls to `self.make_block_same_class` (there are not too many of them I think), if you prefer.

> e.g. pyarrow uses make_block directly

But this is `self.make_block_same_class` which is probably OK to do?
> But this is self.make_block_same_class which is probably OK to do?

Yah, looks like pyarrow is only calling the make_block function, not the methods.
thanks @jorisvandenbossche
ENH: allow object-dtype Index to do arithmetic ops
State: closed

Implement ops in the base Index class to work the same as Series versions.

Labels:
name: Enhancement
name: Algos
description: Non-arithmetic algos: value_counts, factorize, sorting, isin, clip, shift, diff
name: Index
description: Related to the Index class or subclasses

Comments:
I seem to remember an issue about this (which this PR would close), but can't directly find it.

This needs some more tests?

There are also some definitions of arithmetic ops that might no longer be needed?

https://github.com/pandas-dev/pandas/blob/c0ff67a22df9c18da1172766e313732ed2ab6c30/pandas/core/indexes/base.py#L2331-L2354
Can you give some context for closing?
clearing the queue until I can address the comments properly
DOC: Add CoC to the README
State: closed

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


Labels:
name: Docs

Comments:
@sparalic can you merge master & repush? Looks like there was an unrelated CI failure.
@datapythonista I explicitly pushed the file I updated, and did a `git diff upstream/master` to confirm the changes. Is that wrong? Judging on below it seem as if I made it worse.
Not sure what was the problem. You can have a look at the [Files changed](https://github.com/pandas-dev/pandas/pull/27780/files) tab, and you'll see that there are 4 files that are unrelated to what you're changing, and that we don't want to merge into the repo.

Also, I think you reverted some of the changes you already made from a review. We will check later what went wrong, for now you can delete the files in your branch, and leave exactly what you want to get merged in pandas, and then push the branch again.
@sparalic : FYI, if you're having issues pushing, we can definitely help you out instead of your closing the PR.
Avoid calling S3File.s3
State: closed

When reading from S3 using fastparquet. This attribute was removed in
s3fs 0.3.0. This change avoids accessing it by using a new method
`get_file_and_filesystem` which returns the filesystem in addition to the
file.

- [x] closes #27756
- [ ] tests added / passed
- [x] passes `black pandas`
- [x] passes `git diff upstream/master -u -- "*.py" | flake8 --diff`
- [x] whatsnew entry


Labels:
name: IO Data
description: IO issues that don't fit into a more specific label
name: IO Parquet
description: parquet, feather

Comments:
Can you verify that we have CI coverage for s3fs 0.2.x and 0.3.x? Check in `ci/deps/environment*.yml`.
@TomAugspurger here are the references to s3fs there:
```
ci/deps/travis-37.yaml
20:  - s3fs

ci/deps/azure-windows-37.yaml
21:  - s3fs

ci/deps/travis-36-slow.yaml
21:  - s3fs

ci/deps/travis-36-locale.yaml
29:  - s3fs=0.0.8

ci/deps/azure-37-locale.yaml
22:  - s3fs

ci/deps/azure-36-locale_slow.yaml
22:  - s3fs

ci/deps/travis-36-cov.yaml
32:  - s3fs
```
Should we specify versions for some of them?
The tests failures on CI don't look like they are related to this change. I'm also confused why the codecov checks are failing.

Should I add a whatsnew entry for this? In the 0.25.1 file?

Thanks!
You can ignore codecov.

Whatsnew can go in 0.25.1.rst.

We just ensure that we have at least one with the minimum pin. 

As long as we have a test with both s3fs 0.3.1 and pyarrow, so that this test is hit in CI, then I think we're good.
@CJStadler are we hitting this with any tests? Given that our CI wasn't failing, I suspect we'll need a new test to ensure that we're hitting the relevant code path.
@TomAugspurger the lines are covered so I don't think a new test is needed – CI must not have run with one of the affected versions of s3fs. On my machine `TestParquetFastParquet.test_s3_roundtrip` fails on master for s3fs 0.3.0-0.3.2 and passes on my branch.

It looks like s3fs just released 0.3.3 which resolves this, and that test now passes on master.


Yep. Most of these should be simple. There is a FilePathOrBuffer union type you can import from pandas._typing as well

Sent from my iPhone

> On Aug 8, 2019, at 9:24 AM, Chris Stadler <notifications@github.com> wrote:
> 
> @CJStadler commented on this pull request.
> 
> In pandas/io/s3.py:
> 
> > @@ -14,17 +14,15 @@ def _strip_schema(url):
>      return result.netloc + result.path
>  
>  
> -def get_filepath_or_buffer(
> -    filepath_or_buffer, encoding=None, compression=None, mode=None
> -):
> +def get_file_and_filesystem(filepath_or_buffer, encoding=None, mode=None):
> Cool, I didn't realize pandas was using type annotations.
> 
> —
> You are receiving this because you commented.
> Reply to this email directly, view it on GitHub, or mute the thread.

OK by me @TomAugspurger 
Thanks @CJStadler!
BUG: _can_use_numexpr fails when passed large Series
State: closed

This fixes a regression introduced in #27145 where _can_use_numexpr would fail if passed a Series and not a DataFrame. I decided not to use run_arithmetic in the test_suite because there is a separate issue when running floordiv that is out of the scope of the fix. I will open a separate issue in improving the test coverage of "test_expressions.py" as it currently does not check any of the arguments you can pass to the operators, such as 'axis', 'level' and 'fill_value'.

- [x] closes #27636 
- [x] tests added / passed
- [x] passes `black pandas`
- [x] passes `git diff upstream/master -u -- "*.py" | flake8 --diff`
- [x] whatsnew entry


Labels:
name: Regression
description: Functionality that used to work in a prior pandas version
name: Numeric Operations
description: Arithmetic, Comparison, and Logical operations

Comments:
There was some CI failures (Looked like some unrelated http exceptions so I pulled the latest commits in from master to see if that fixes it). 

I have added the xfail for axis=1 on floordiv and made the whatsnew entry more specific now which should now cover all comments.
I think the CI error is unrelated.  Can you rebase and see if it resolves itself?
> I think the CI error is unrelated. Can you rebase and see if it resolves itself?

That has fixed it. @WillAyd are you comfortable with the changes?
Hi @WillAyd is the Whatsnew entry ok now?
FYI, can you just merge master & do a regular push, rather than a force push? We don't always get notifications on force pushes.
This should also close #27853 @ccharlesgb can you make sure the use case there is covered by the new tests?
> This should also close #27853 @ccharlesgb can you make sure the use case there is covered by the new tests?

@jbrockmendel yes this case is covered in the test as long as divide/div and truediv are all still aliases of each other?
> FYI, can you just merge master & do a regular push, rather than a force push? We don't always get notifications on force pushes.

@jbrockmendel Had to rebase as the CI was failing again. But I think everything is addressed now.
Thanks @ccharlesgb!
REF: Make CategoricalIndex comparison defer to Categorical comparison
State: closed

Partially addresses #19513.

After this, CategoricalIndex will be defining comparison ops identically to DTA/TDA/PA, could share some code.

Labels:
name: API Design
name: Numeric Operations
description: Arithmetic, Comparison, and Logical operations
name: Categorical
description: Categorical Data Type

Comments:
looks fine; are the added paths  hit in coverage? e.g. do we need any additional tests?
The new paths are all hit, yes.
thanks @jbrockmendel