CLN remove unreachable code in pandas/core/groupby/generic.py::DataFrameGroupBy::_wrap_applied_output

[MarcoGorelli]

NOTE: I'd originally opened a PR to address this, but am now busy with other obligations + other PRs to respond to. So I'm opening it up as a good first issue for now, and will return to work on it if nobody takes it.

---

In pandas/core/groupby/generic.py::DataFrameGroupBy::_wrap_applied_output, it seems this branch is never reached:

        else:
            # Handle cases like BinGrouper
            return self._concat_objects(keys, values, not_indexed_same=not_indexed_same)

Two things need to be done here:

1. refactor _wrap_applied_output by making a helper function out of some of its internals (it's currently very long, and changes are hard to review)
2. Remove this unused else branch

[sarthakvk]

Hello @MarcoGorelli, I would like to work on this, can you elaborate 1 point that needs to be done.

[alimcmaster1]

I think the first steps are to look at the _transform_general function and see how it could be broken down in more manageable/reusable functions. Might be quite a challenging good first issue - but feel free to give it a shot and let us know if you have any questions

[MarcoGorelli]

Yes, that's right - first step would be to break down this long function into smaller parts. After that (as a separate PR) you could simply remove the unused else branch.

Might be quite a challenging good first issue

True - but yes, do feel free to ask questions, and refer to the contributing guide for how to get started

[sarthakvk]

Ok @MarcoGorelli, I will try my best 😄

[Sylfrena]

hey @sarthakchaudhary13 are you still working on this? if you are busy and don't have time for this issue anymore I would like to pick it up. Let me know 😄

In pandas/core/groupby/generic.py::DataFrameGroupBy::_transform_general, it seems this branch is never reached:

    else:
        # Handle cases like BinGrouper
        return self._concat_objects(keys, 

@MarcoGorelli here you have mentioned this else block as a part of the _transform_general function but I went through the codebase and found that the else block is actually a part of _wrap_applied_output and I couldn't find any calls to _wrap_applied_output in _transform_general or vice versa

I also went through your closed PR and noticed that most of the refactoring has been done _wrap_applied_output method and _transform_general was left untouched. So I am a bit confused here. If you could clarify this it would be a great help.

Thanks.

[MarcoGorelli]

@Sylfrena yes, you're right, it's in _wrap_applied_output, not _transform_general - thanks!

[sarthakvk]

hello @Sylfrena, Yeah sure give it a go 😃

[kehv1n]

Hey everyone, I can gladly take responsibility for this issue if @Sylfrena hasn't finished yet.

I am currently setting up my developer env, but will follow up here when I finish to see if this needs to be resolved.

Thanks

[MarcoGorelli]

Hi @kehv1n - TBH, given the silence, I'd assume nobody is working on it, so if you'd like to submit a PR it'd be welcome. Feel free to ask on the gitter channel if you have any issues setting up your development environment

[Sylfrena]

@kehv1n yes, please go ahead with the issue.

[kehv1n]

take

[kehv1n]

I apologize for the delay, question. @MarcoGorelli originally mentioned refactoring portions of wrap_applied_output into helper functions. That is, I should abstract out some of these if statements into external, properly named functions?

Including the unreachable else branch, there are a total of 5 parent conditionals.

First time contributing, I apologize for the confusion.

[MarcoGorelli]

Hi @kehv1n - yes, that's right! Feel free to ping if you're stuck on something

[FollowTheProcess]

Hi @kehv1n are you still working this? I'd like to help out if I can

[MarcoGorelli]

@FollowTheProcess given the silence you're probably OK to take this if you want to work on it

[FollowTheProcess]

@MarcoGorelli Thanks! I've removed the unreached else and no change in local tests before/after. The tidying of the enclosing function might take be a bit longer, I'm not entirely sure how to do anything helpful with it! I'll keep playing. In the meantime would it be a good idea to do (my first ever) PR for the removal of the unreached else?

[MarcoGorelli]

@FollowTheProcess sure, if it's your first contribution maybe it's easier to just remove the unused else branch for now, and then there can be separate PR to refactor this code, and finally one to remove the elif above it and dedent the block which comes after it

[FollowTheProcess]

@MarcoGorelli Cool, thanks for the help! I'll do the PR now

[FollowTheProcess]

I didn't mean to say it would close the issue! sorry, didn't realise it would do that automatically if I typed in the issue number

[fokoid]

Sounds like the refactoring part of this is still to be done. I can take a look if no one is working on it anymore.

[MarcoGorelli]

Hi @fokoid , thanks for bringing my attention back to this - it looks like @rhshadrach has already done some excellent refactoring here, this function is now significantly simpler than it was when I opened this, so IMO we're OK to close

Nonetheless, there are many open issues to work on, any help there would be welcome!