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.