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

Define plot methods in class definitions #16913

Closed
wants to merge 3 commits into from

Conversation

jbrockmendel
Copy link
Member

instead of pinning them to Series/DataFrame at the bottom of the files.

(For reasons I dont quite get) Importing pandas.plotting._core at the top of core.series
instead of near the bottom causes a circular-import problem. Avoided this by delaying
import from core.series inside plotting._tools and plotting._core.

Follows-up on conversation in #11053

instead of pinning them to Series/DataFrame at the bottom of the files.

(For reasons I dont quite get) Importing pandas.plotting._core at the top of core.series
instead of near the bottom causes a circular-import problem.  Avoided this by delaying
import from core.series inside plotting._tools and plotting._core.
@jbrockmendel
Copy link
Member Author

Am I understanding correctly that the point of ABCSeries is to be able to effectively check isinstance(foo, Series) without having to import Series? If so, I'll amend this PR to avoid the circular import more gracefully.

@gfyoung
Copy link
Member

gfyoung commented Jul 13, 2017

Am I understanding correctly that the point of ABCSeries is to be able to effectively check isinstance(foo, Series) without having to import Series?

Yes, that is correct. I think you will need to find a way to work around the circular dependency that you found without importing Series in so many places.

@jbrockmendel
Copy link
Member Author

I think you will need to find a way to work around the circular dependency that you found without importing Series in so many places.

Great, thanks. Just changed those to check against ABCSeries and ABCDataFrame. Much prettier.

@gfyoung
Copy link
Member

gfyoung commented Jul 14, 2017

@jbrockmendel : I was able to read more on the issue you referenced, and it occurs to me: why are you refactoring these methods? IIUC, the consensus was to just deprecate these methods. While refactoring may make things a little nicer from an organizational standpoint, IMO you should probably just leave it alone and deprecate immediately.

Note: that would also allow you to not have to figure out why all of these tests are currently failing 😄

@jbrockmendel
Copy link
Member Author

and it occurs to me: why are you refactoring these methods?

As a newcomer, I experienced a good deal of frustration in trying to figure out e.g. "where the heck is Series.ptp defined??" I developed a strong preference for methods/attributes being defined in the first-place-you-look kind of way. In the case of the plot, hist, and boxplot, defining them inside the class DataFrame... block seems like the "one obvious way to do it".

Also in many of these cases there is a good reason for the design decision, in which case it's a learning opportunity. (Thanks jreback!)

Note: that would also allow you to not have to figure out why all of these tests are currently failing

... [grumble]

@gfyoung
Copy link
Member

gfyoung commented Jul 14, 2017

As a newcomer, I experienced a good deal of frustration in trying to figure out e.g. "where the heck is Series.ptp defined??" I developed a strong preference for methods/attributes being defined in the first-place-you-look kind of way. In the case of the plot, hist, and boxplot, defining them inside the class DataFrame... block seems like the "one obvious way to do it".

That is fair, but as these methods are being deprecated (correct me if I'm wrong), why aren't you using the preferred methods for invoking these functions? Using soon-to-be deprecated methods (you could be the one who deprecates it 😉 ) is generally not advised.

If there are methods that you find frustrating to find, then by all means attempt to refactor those, but I'm still not convinced that you should be refactoring methods that are going to be deprecated.

@gfyoung
Copy link
Member

gfyoung commented Jul 14, 2017

Also, I should add that many of these methods (like ptp) were defined with the purpose of making them template-able across multiple functions and classes. You can witness their inner-workings from the core/generic.py file in case you're interested.

@jbrockmendel
Copy link
Member Author

jbrockmendel commented Jul 14, 2017 via email

@gfyoung
Copy link
Member

gfyoung commented Jul 14, 2017

The thought (evidence to the contrary) was that a PR that doesn't change any logic wouldn't get the same pushback as something more ambitious.

  1. The tests broke with your initial commit, which indicates that some logic actually did get changed.

  2. In general yes, but the benefit of refactoring code when it is going to get deprecated (and eventually deleted) is very much diminished. In addition, if you are interested in getting a PR merged, you should try to implement changes that have the support of the maintainers of the repository.

That's why I suggested you deprecate those two functions in your PR. You already have the backing of several maintainers judging from the issue, and I for one am in agreement with that decision. Thus, I would be happy to help you through the process of deprecation if that interests you.

@jbrockmendel
Copy link
Member Author

The tests broke with your initial commit, [...]

Tests broke because I didn't import ABCIndex, should now be fixed.

[...] which indicates that some logic actually did get changed.

Notwithstanding the stuff I managed to break, avoiding the fragility of the circular-ish import between plotting._core and core.series is probably a more important fix in itself than the original goal.

That's why I suggested you deprecate those two functions in your PR.

OK. Does this just mean removing hist and boxplot from DataFrame, and hist from Series?

@jbrockmendel
Copy link
Member Author

Does this just mean removing hist and boxplot from DataFrame, and hist from Series?

If so, then I'm going to push for this to be a separate PR. Removing those from the namespace then requires changes in core.groupby, which I don't want to touch until I get a handle on _whitelist_method_generator and why exec is needed.

@gfyoung
Copy link
Member

gfyoung commented Jul 14, 2017

Tests broke because I didn't import ABCIndex, should now be fixed.

Unfortunately, that does not appear to be the case. In fact, I think the current test failures are the same ones I saw on your initial commit. 😢

avoiding the fragility of the circular-ish import between plotting._core and core.series is probably a more important fix in itself than the original goal.

That is a fair point. That might go away once those hist and plot methods are removed.

Does this just mean removing hist and boxplot from DataFrame, and hist from Series?

No. Deprecating means that we issue a FutureWarning to users if they call the method, warning them that the method will be removed in the future. We'll save the removal for another time. 😄

BTW, if you want to see how the deprecation cycle works, have a look at #13777. The PR's to the right (on each bullet point) are the ones where the code got deprecated, and the PR's to the left are the ones where the code got removed.

That being said, if you would like to contribute more, #6581 lists the current functions that have yet to be removed for this release (0.21). Those I guarantee will get no push-back!

@jbrockmendel
Copy link
Member Author

Unfortunately, that does not appear to be the case. In fact, I think the current test failures are the same ones I saw on your initial commit

AFAICT the relevant error message in Travis is:

>       if not isinstance(secondary_y, (bool, tuple, list, np.ndarray, ABCIndex)):
E       NameError: global name 'ABCIndex' is not defined

pandas/plotting/_core.py:161: NameError

The last commit does import ABCIndex at the top of plotting.core, so either the Travis output still reflects the earlier commit or I'm even more confused than before.

That is a fair point. That might go away once those hist and plot methods are removed.

I tried that just for kicks and it opens up a new can of worms. It necessitates edits in core.groupby, which in turn require changes to expected test output. Let's get the circular import fixed here and move on.

@gfyoung
Copy link
Member

gfyoung commented Jul 14, 2017

I tried that just for kicks and it opens up a new can of worms. It necessitates edits in core.groupby, which in turn require changes to expected test output. Let's get the circular import fixed here and move on.

Slow down there 😄 ! Did you read my previous comment? I didn't say that you should remove them immediately. You should be deprecating only (see my comment above - I said we'll save removal for another time). DO NOT REMOVE.

After deprecation, feel free to have a crack at removing that circular import (if possible).

@jbrockmendel
Copy link
Member Author

Did you read my previous comment.

I did. In fact I quoted it to make clear what I was referencing.

After deprecation, feel free to have a crack at removing that circular import (if possible).

The circular import is a Bad And Should Feel Bad. I'll be happy to come back later, get up to speed on the deprecation cycle conventions, and then make a PR for the thing that you care about. But since Travis doesn't want to play along, this will remain broken. Gotta go do bill-paying work. Closing.

@gfyoung
Copy link
Member

gfyoung commented Jul 14, 2017

I tried that just for kicks and it opens up a new can of worms. It necessitates edits in core.groupby, which in turn require changes to expected test output. Let's get the circular import fixed here and move on.

The reason I asked is because this comment doesn't make much sense given that deprecating just involves adding a FutureWarning right under the calling of the function. You shouldn't have to do anything else, unless these functions are getting called elsewhere in the code-base (if they are, do mention that).

@gfyoung
Copy link
Member

gfyoung commented Jul 14, 2017

Gotta go do bill-paying work. Closing.

Understood. Thanks for taking a crack at this. Feel free to address the deprecation when you have the time.

jbrockmendel added a commit to jbrockmendel/pandas that referenced this pull request Jul 14, 2017
This follows up on pandas-dev#16913 but cuts down on the scope of the PR.
@jbrockmendel jbrockmendel deleted the plot_cleanup branch July 21, 2017 18:44
@gfyoung gfyoung modified the milestones: 1.0, No action Aug 8, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants