ENH: Implement Keyword Aggregation for DataFrame.agg and Series.agg
State: closed

- [x] closes #26513, related to #28380 
- [x] tests added / passed
- [x] passes `black pandas`
- [x] passes `git diff upstream/master -u -- "*.py" | flake8 --diff`
- [ ] whatsnew entry

This PR basically deals with three issue:
1. implement keyword aggregation for DataFrame.agg
2.  implement keyword aggregation for Series.agg


Labels:
name: Enhancement
name: Groupby

Comments:
@charlesdong1991 is this still active? Can you address comment from @jreback ?
thanks for your comment @WillAyd 

i indeed tried to shorten the code based on comment, and this is the revised version which passes all cases, probably still not simplified enough. (i remembered i tried to remove some codes, but in some cases, especially related to the order of columns, will fail in tests)

How about splitting it into two PRs with one for Series and one for DataFrame? so that this PR can be more quickly reviewed? 
> so that this PR can be more quickly reviewed?

We appreciate that you've been very patient both here and in other PRs.  You're making some really good contributions, keep it up!
Many thanks for your nice reviews and kind words, I appreciate a lot!! @jbrockmendel 

it's been a while since my last commit and I do need some time to pick up what had done and need to be done following your review. I will try to come up with revised version ASAP, and then pin you! Thanks again!!
@jreback @jbrockmendel 

I have addressed the both of your comments and update the code as well. Feel free to take a look if you have time! Thanks!

Self-review: this looks a bit repeated, however, as I addressed in the comment, after this PR, could do a follow-up PR to replace the duplicates in `generic.py` where named agg is used, and use the function defined in `SelectionMixin`, and afterwards, with `window` and `rolling`, they all would use the function from `SelectionMixin`
Hi, @jreback 

I updated this PR, although it looks quite big, but actually not, it basically includes these steps:
1. Move helper functions to `pandas.core.aggregation.py`
2. Clean the imports
3. Deduplicate the code (the reason to have this `reconstruct_func` method was I found out for frames, the first part of preprocessing is identical, so could use a generic function for them), therefore
4. add the other part of postprocessing/logic for frame and implement logic for series (for frame, the processing seems a bit too much, i recall this was due to some column order issue, espeically for very complex cases(e.g. with lambda, etc), some processing is needed, but this might because back then py35 was there and dict does not preserve order in py35

i would be very looking forward to hearing your feedbacks and hope this could be included in 1.0, thanks!
I think the PR is small enough this time for a detailed review ^^ @jreback  thanks!

Regarding the movement of agg tests to a new test file, i am def on board with it. I have two proposals: 1. I move the tests I have in this PR to new file first and then move other related tests in a follow-up. 2. keep the tests as where they are in this PR, and have a follow-up PR to move all agg tests to new test files.

I think either of them could make the scope of this PR clearer and not too big. I would be very happy to hear your opinions. thanks! 
also just in case if you'd have time for a review ^^ @WillAyd  thanks!
this is not urgent PR, some followup reviews would be appreciated @jreback ^^
@jreback since simply creating a file/subdir does not make too much sense, and moving the tests around is not a big mess to this PR in terms of the code change, so let's do it all in one go!
any followups? @WillAyd @jreback 

thanks a lot!
@charlesdong1991 this got a bit lost, can you merge master and will look
thanks for picking this up back @jreback 

Feel free to provide your opinions, and I will make changes accordingly together with @WillAyd previous reviews
Hello @charlesdong1991! 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-07-10 16:53:51 UTC
maybe let's also try to push this PR once again? thanks! @jreback @WillAyd  ^^

let me briefly summarize since it's been here for a while:
1. added a `reconstruct_func` method which just copied pasted the part from `generic.py`, and this func is used to reconstruct funcs if relabelling.
2. added a `relabel_result` method to reorder the aggregated result and put the right label for them as the final aggregated return. The reason to return a dictionary for this function and then construct the DataFrame afterwards in `frame.py` is to avoid circular imports in `aggregation.py`.
3. removed the part from `generic.py` which becomes `reconstruct_func` to reduce code repeats

Your feedbacks to push it again will be very appreciated and apologies for making it not very readable and hard to review. Thus any suggestions would be very welcome!! thanks again!
@charlesdong1991 this one has slipped the queue, can you merge master and take another look
sure, will try to address all comments soon
emm, CI failure seems irrelevant.
some further feedbacks are very welcomed! @jreback @WillAyd 
ping @jreback @WillAyd  for further reviews, thanks in advance!
oh, sorry, i am working on the annotations, just haven't pushed the commit @jreback 

will ping you for review once i have simplified annotations!
emm, the errors look unrelated to this PR, I will rebase once there is new commits merged to master.

In the meantime, maybe pls take a look at the annotations so that i could make adjustments if needed? @jreback 
ehh?? looks good again somehow? 😅   i thought i saw errors back then

sorry for the noise 
thanks @charlesdong1991 very nice!