Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Untangle reshape imports --> Define pivot_table in DataFrame #17215

Closed
wants to merge 2 commits into from

Conversation

jbrockmendel
Copy link
Member

This is an alternative to #17174. This is ultimately a better solution to the issue there, but involves a more invasive refactoring.

To recap:

  • Right now DataFrame.pivot_table is defined in core.reshape.pivot with DataFrame.pivot_table = pivot_table. The original and main goal of this (and move pivot_table doc-string to DataFrame #17174) is to avoid that, and instead define DataFrame.pivot_table within the DataFrame class definition.

  • Doing this makes the docstrings tricky, since we want pivot_table.__doc__ to be shared by reshape.pivot.pivot_table and DataFrame.pivot_table. To resolve this cleanly, we need to import reshape.pivot at the top of core.frame. Doing this requires getting the import of DataFrame out of reshape.pivot (and anything else it imports, namely reshape.concat).

Bonus: If/when this is approved, all of the mid-module imports of reshape.concat can be moved to the top of core.frame, and a handful of other docstrings can be put into _shared_docs in more convenient locations.

Bonus 2: If/when this is approved, doing the same thing in reshape.merge and reshape.tile is very easy. reshape.reshape is doable but take a bit more work.

  • closes #xxxx
  • tests added / passed
  • passes git diff upstream/master -u -- "*.py" | flake8 --diff
  • whatsnew entry

so that core.frame can import these safely at the top of the module.  This allows us to
define DataFrame.pivot_table inside DataFrame instead of pinning it on from reshape.pivot

This also lets us put docstrings in _shared_docs adjacent to the implementations without
having to contort ourselves to make sure imports are ordered correctly.
@gfyoung gfyoung added the Clean label Aug 10, 2017
@codecov
Copy link

codecov bot commented Aug 10, 2017

Codecov Report

Merging #17215 into master will decrease coverage by 0.04%.
The diff coverage is 97.36%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #17215      +/-   ##
==========================================
- Coverage   91.02%   90.98%   -0.05%     
==========================================
  Files         162      162              
  Lines       49517    49539      +22     
==========================================
  Hits        45075    45075              
- Misses       4442     4464      +22
Flag Coverage Δ
#multiple 88.77% <97.36%> (-0.03%) ⬇️
#single 40.26% <47.36%> (-0.07%) ⬇️
Impacted Files Coverage Δ
pandas/core/reshape/concat.py 97.67% <100%> (+0.04%) ⬆️
pandas/core/frame.py 97.72% <100%> (-0.1%) ⬇️
pandas/core/reshape/pivot.py 95.16% <95%> (-0.15%) ⬇️
pandas/io/gbq.py 25% <0%> (-58.34%) ⬇️
pandas/plotting/_converter.py 63.23% <0%> (-1.82%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f165b90...2299fd2. Read the comment docs.

@codecov
Copy link

codecov bot commented Aug 10, 2017

Codecov Report

Merging #17215 into master will decrease coverage by 0.04%.
The diff coverage is 97.36%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #17215      +/-   ##
==========================================
- Coverage   91.02%   90.98%   -0.05%     
==========================================
  Files         162      162              
  Lines       49517    49539      +22     
==========================================
  Hits        45075    45075              
- Misses       4442     4464      +22
Flag Coverage Δ
#multiple 88.77% <97.36%> (-0.03%) ⬇️
#single 40.26% <47.36%> (-0.07%) ⬇️
Impacted Files Coverage Δ
pandas/core/reshape/concat.py 97.67% <100%> (+0.04%) ⬆️
pandas/core/frame.py 97.72% <100%> (-0.1%) ⬇️
pandas/core/reshape/pivot.py 95.16% <95%> (-0.15%) ⬇️
pandas/io/gbq.py 25% <0%> (-58.34%) ⬇️
pandas/plotting/_converter.py 63.23% <0%> (-1.82%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f165b90...2299fd2. Read the comment docs.

@codecov
Copy link

codecov bot commented Aug 10, 2017

Codecov Report

Merging #17215 into master will decrease coverage by 0.04%.
The diff coverage is 97.36%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #17215      +/-   ##
==========================================
- Coverage   91.02%   90.98%   -0.05%     
==========================================
  Files         162      162              
  Lines       49517    49539      +22     
==========================================
  Hits        45075    45075              
- Misses       4442     4464      +22
Flag Coverage Δ
#multiple 88.77% <97.36%> (-0.03%) ⬇️
#single 40.26% <47.36%> (-0.07%) ⬇️
Impacted Files Coverage Δ
pandas/core/frame.py 97.72% <100%> (-0.1%) ⬇️
pandas/core/reshape/concat.py 97.67% <100%> (+0.04%) ⬆️
pandas/core/reshape/pivot.py 95.16% <95%> (-0.15%) ⬇️
pandas/io/gbq.py 25% <0%> (-58.34%) ⬇️
pandas/plotting/_converter.py 63.23% <0%> (-1.82%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f165b90...2299fd2. Read the comment docs.

@jreback
Copy link
Contributor

jreback commented Aug 11, 2017

I like the other soln better. This is going backwards.

@jreback jreback closed this Aug 11, 2017
@jbrockmendel jbrockmendel deleted the reshape_imports branch October 30, 2017 16:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants