Move GroupBy to Submodule and Add FutureWarning #20506

WillAyd · 2018-03-27T17:04:15Z

ref #20485 @jreback

WillAyd · 2018-03-27T17:05:09Z

pandas/tests/api/test_api.py

+        "Grouper", "groupby", "BinGrouper", "Grouper", "_GroupBy", "GroupBy",
+        "SeriesGroupBy", "_pipe_template", "PanelGroupBy", "Grouping",
+        "SpecificationError", "DataError", "generate_bins_generic"])
+    def test_groupby_move(self, attr):


This would be straightforward to combine with the method above if we wanted one test accepting a module / list of objects to test the deprecation for

WillAyd · 2018-03-28T06:08:34Z

There was a failure in one of the "Allowed Failures" as a result of a ResourceWarning for an open TextIOBuffer (?). I don't believe that is related to this change and it didn't appear in any of the previous CI runs

jreback · 2018-03-27T17:14:48Z

pandas/core/groupby/__init__.py

+              stacklevel=2)
+
+from pandas.core.groupby.groupby import Grouper  # noqa: F401
+from pandas.core.groupby.groupby import groupby  # noqa: F401


can u import these in 1 statement

jreback · 2018-03-27T17:15:45Z

pandas/core/groupby/__init__.py

+from pandas.core.groupby.groupby import GroupBy  # noqa: F401
+from pandas.core.groupby.groupby import SeriesGroupBy  # noqa: F401
+from pandas.core.groupby.groupby import _pipe_template  # noqa: F401
+from pandas.core.groupby.groupby import PanelGroupBy  # noqa: F401


are all these imports actually needed?

just change where they r used

jreback · 2018-03-28T10:09:47Z

There was a failure in one of the "Allowed Failures" as a result of a ResourceWarning for an open TextIOBuffer (?). I don't believe that is related to this change and it didn't appear in any of the previous CI runs

FYI there are NO allowed failures, meaning that yes these will allow the build to complete earlier, but generally we don't merge things if they are causing issues.

jreback · 2018-03-28T10:11:21Z

pandas/core/groupby/__init__.py

+
+from pandas.core.groupby.groupby import Grouper  # noqa: F401
+from pandas.core.groupby.groupby import groupby  # noqa: F401
+from pandas.core.groupby.groupby import BinGrouper  # noqa: F401


only put public functions here. If something is needed internally then change the reference to it.

codecov · 2018-03-28T11:38:15Z

Codecov Report

Merging #20506 into master will increase coverage by 0.02%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master   #20506      +/-   ##
==========================================
+ Coverage   91.82%   91.84%   +0.02%     
==========================================
  Files         152      153       +1     
  Lines       49255    49256       +1     
==========================================
+ Hits        45226    45239      +13     
+ Misses       4029     4017      -12

Flag	Coverage Δ
#multiple	`90.23% <100%> (+0.02%)`	⬆️
#single	`41.9% <66.66%> (ø)`	⬆️

Impacted Files	Coverage Δ
pandas/core/groupby/groupby.py	`92.55% <ø> (ø)`
pandas/core/reshape/pivot.py	`96.97% <100%> (ø)`	⬆️
pandas/core/resample.py	`96.43% <100%> (ø)`	⬆️
pandas/core/api.py	`100% <100%> (ø)`	⬆️
pandas/core/groupby/__init__.py	`100% <100%> (ø)`
pandas/core/generic.py	`95.94% <100%> (ø)`	⬆️
pandas/core/panel.py	`97.29% <100%> (ø)`	⬆️
pandas/plotting/_converter.py	`66.81% <0%> (+1.73%)`	⬆️

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 4efb39f...7624313. Read the comment docs.

TomAugspurger · 2018-03-28T16:12:43Z

FYI, merge conflict with master.

WillAyd · 2018-03-29T16:10:50Z

Hmm so I'm not sure that the FutureWarning being in the __init__.py of the groupby package is ideal here because the internal references point to pandas.core.groupby.groupby. I think warning in that init will always get thrown, because the new module is part of the package where that __init__.py exists, so it would always get loaded?

pandas.core.categorical didn't have this problem because the updated reference was higher in the package hierarchy. The deprecated module doesn't get loaded when using the newer reference. Thinking through what our options here are but open to ideas

jorisvandenbossche · 2018-03-29T21:33:49Z

Can we just import what is needed in __init__.py? Then no deprecation warning is needed (python doesn't care if it is a single file module or a package directory, so we can make use of that).
Personally I would even also use those imports internally. Then you don't need to change all the imports in the rest of pandas, if you reorganize things internally within the groupby subpackage.

WillAyd · 2018-03-29T22:08:23Z

@jorisvandenbossche I already updated all the internal refs per Jeff's previous comment, but could undo if required. That said, maybe we just shouldn't put the warning on the init given the change could be transparent from an API perspective since all we are doing is moving from a single module to a like-named package?

WillAyd · 2018-03-30T01:45:47Z

pandas/core/groupby/__init__.py

+              "'pandas.core.groupby.groupby' instead", FutureWarning,
+              stacklevel=2)
+
+from pandas.core.groupby.groupby import (Grouper, GroupBy,  # noqa: F401


GroupBy, SeriesGroupBy and DataFrameGroupBy were all added here as they were causing downstream failures for Dask, even though they aren't explicitly referenced internally. Once we settle on a go-forward here we'll have to follow up with the Dask team to let them know of impacts

yes this is fine. These are basically downstream dev dependencies (e.g. you may want to do something based on the object if its a SeriesGroupBy or a DataFrameGroupBy) so ok with this

jorisvandenbossche · 2018-03-30T07:57:42Z

That said, maybe we just shouldn't put the warning on the init given the change could be transparent from an API perspective since all we are doing is moving from a single module to a like-named package?

Yes, that's exactly what I tried to say :-)

IMO, this is much cleaner (also internally, but certainly for external packages)

jreback · 2018-03-30T19:42:42Z

pandas/core/groupby/__init__.py

+              "'pandas.core.groupby.groupby' instead", FutureWarning,
+              stacklevel=2)
+
+from pandas.core.groupby.groupby import (Grouper, GroupBy,  # noqa: F401


yes this is fine. These are basically downstream dev dependencies (e.g. you may want to do something based on the object if its a SeriesGroupBy or a DataFrameGroupBy) so ok with this

jreback · 2018-03-30T19:43:14Z

pandas/core/resample.py

-from pandas.core.groupby import (BinGrouper, Grouper, _GroupBy, GroupBy,
-                                 SeriesGroupBy, groupby, PanelGroupBy,
-                                 _pipe_template)
+from pandas.core.groupby.groupby import (BinGrouper, Grouper, _GroupBy,


style nit, I like to see a newline after the parens so these are wrapped a bit

jreback · 2018-03-30T19:44:22Z

very tiny style issue, ping on green.

jorisvandenbossche · 2018-03-30T19:56:16Z

@jreback did you see our discussion above?

So -1 on merge on green. There is no need to deprecate this (and we can still move to a subpackage)

jreback · 2018-03-30T19:59:06Z

@jorisvandenbossche sorry, I did see that. yeah we can not deprecate is fine.

WillAyd · 2018-03-31T01:43:39Z

Does anyone develop on Windows? Trying to figure out why CircleCI is failing but can't reproduce the issue locally.

An easy way to solve it would be to add BinGrouper back to the API in the __init__.py but that doesn't seem required for anything else internal, so figure it's best to figure out what is different about the Windows import mechanism here

jorisvandenbossche · 2018-04-01T12:11:42Z

CircleCI is also linux, not windows. But why it is failing there and not in the others, I don't know.
But you can always import from pandas. .... import BinGrouper and then test against that, that should certainly work.

I would also like to advocate to keep the internal imports the same as before (so import into __init__.py everything that is "public" for the rest of pandas)

WillAyd · 2018-04-01T15:03:19Z

@jreback what do you think about Joris' comment? Should I put everything back into the __init__.py and undo the changes to internal references or simply update this last failing test case?

jreback · 2018-04-01T22:40:06Z

i would move non public this out of the init

jreback · 2018-04-02T22:38:37Z

thanks @WillAyd

`pandas.core.groupby` has been moved to `pandas.core.groupby.groupby` for pandas 0.23.0. See pandas pull request [#20506](pandas-dev/pandas#20506).

WillAyd added 3 commits March 27, 2018 09:45

Added test for FutureWarning

8eff7db

Moved pandas.core.groupby to submodule

37e5f1c

LINT fixup

6bd1301

WillAyd commented Mar 27, 2018

View reviewed changes

WillAyd added 2 commits March 27, 2018 16:48

Merge remote-tracking branch 'upstream/master' into grpby-sub-mod

2485a15

Added DataFrameGroupBy to module-level api

ec8395f

jreback reviewed Mar 28, 2018

View reviewed changes

jreback requested changes Mar 28, 2018

View reviewed changes

jreback added Refactor Internal refactoring of code Groupby labels Mar 28, 2018

WillAyd added 5 commits March 28, 2018 20:25

Merge remote-tracking branch 'upstream/master' into grpby-sub-mod

884850a

Updated internal references to groupby

9acba5c

Merge remote-tracking branch 'upstream/master' into grpby-sub-mod

a07fe88

Cleaned up API and tests

1f20857

Cleaned up BinGrouper test reference

7d29785

WillAyd added 2 commits March 29, 2018 14:20

Dask support

2b2d59d

Merge remote-tracking branch 'upstream/master' into grpby-sub-mod

0f01b4c

WillAyd commented Mar 30, 2018

View reviewed changes

jreback approved these changes Mar 30, 2018

View reviewed changes

jreback added this to the 0.23.0 milestone Mar 30, 2018

jsexauer mentioned this pull request Mar 30, 2018

DEPR: Clean up list of deprecations from prior versions #6581

Closed

1 task

WillAyd added 5 commits April 1, 2018 17:55

Merge remote-tracking branch 'upstream/master' into grpby-sub-mod

8867133

Reverted deprecation

97f349a

Added explicit import to fix test

db4abbe

Updated import formatting

da6f676

Changed import ref to use full pandas name

7624313

jreback merged commit 5edc5c4 into pandas-dev:master Apr 2, 2018

WillAyd deleted the grpby-sub-mod branch May 14, 2018 21:11

toddrme2178 mentioned this pull request May 17, 2018

Support pandas 0.23.0 core.groupby module layout tqdm/tqdm#554

Closed

casperdcl pushed a commit to tqdm/tqdm that referenced this pull request May 21, 2018

Support pandas 0.23.0 core.groupby module layout

39f8d61

`pandas.core.groupby` has been moved to `pandas.core.groupby.groupby` for pandas 0.23.0. See pandas pull request [#20506](pandas-dev/pandas#20506).

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move GroupBy to Submodule and Add FutureWarning #20506

Move GroupBy to Submodule and Add FutureWarning #20506

WillAyd commented Mar 27, 2018

WillAyd Mar 27, 2018

WillAyd commented Mar 28, 2018

jreback Mar 27, 2018

jreback Mar 27, 2018

jreback commented Mar 28, 2018

jreback Mar 28, 2018

codecov bot commented Mar 28, 2018 •

edited

Loading

TomAugspurger commented Mar 28, 2018

WillAyd commented Mar 29, 2018

jorisvandenbossche commented Mar 29, 2018

WillAyd commented Mar 29, 2018

WillAyd Mar 30, 2018

jreback Mar 30, 2018

jorisvandenbossche commented Mar 30, 2018

jreback Mar 30, 2018

jreback Mar 30, 2018

jreback commented Mar 30, 2018

jorisvandenbossche commented Mar 30, 2018

jreback commented Mar 30, 2018

WillAyd commented Mar 31, 2018

jorisvandenbossche commented Apr 1, 2018

WillAyd commented Apr 1, 2018

jreback commented Apr 1, 2018

jreback commented Apr 2, 2018

Move GroupBy to Submodule and Add FutureWarning #20506

Move GroupBy to Submodule and Add FutureWarning #20506

Conversation

WillAyd commented Mar 27, 2018

Choose a reason for hiding this comment

WillAyd commented Mar 28, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jreback commented Mar 28, 2018

Choose a reason for hiding this comment

codecov bot commented Mar 28, 2018 • edited Loading

Codecov Report

TomAugspurger commented Mar 28, 2018

WillAyd commented Mar 29, 2018

jorisvandenbossche commented Mar 29, 2018

WillAyd commented Mar 29, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jorisvandenbossche commented Mar 30, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jreback commented Mar 30, 2018

jorisvandenbossche commented Mar 30, 2018

jreback commented Mar 30, 2018

WillAyd commented Mar 31, 2018

jorisvandenbossche commented Apr 1, 2018

WillAyd commented Apr 1, 2018

jreback commented Apr 1, 2018

jreback commented Apr 2, 2018

codecov bot commented Mar 28, 2018 •

edited

Loading