pynapple-org / pynapple

PYthon Neural Analysis Package :pineapple:
https://pynapple-org.github.io/pynapple/
MIT License
243 stars 59 forks source link

[ENH] Add TsGroup.merge_group method #275

Closed qian-chu closed 1 month ago

qian-chu commented 1 month ago

Implements https://github.com/pynapple-org/pynapple/issues/251.

Added a static method TsGroup.merge_group() as well as a tsgroup.merge(). Documentation with examples is included as well.

qian-chu commented 1 month ago

Hi Qian-Chu. Thanks for your contribution, this is great. Very neat docstrings, and well thought out method. I would ask you a couple of edits before we can merge this:

  1. Run linters and formatters
black pynapple/core/ts_group.py
isort --check pynapple --profile black
flake8 pynapple --max-complexity 10    
  1. Add tests of your method hitting all the exceptions. you can use @pytest.mark.parametrize to loop over multiple tsgroup and kwargs configs.

Cheers, Edoardo

Will do! Thanks for reviewing and providing feedback!

coveralls commented 1 month ago

Pull Request Test Coverage Report for Build 9151076635

Details


Changes Missing Coverage Covered Lines Changed/Added Lines %
pynapple/core/ts_group.py 38 40 95.0%
<!-- Total: 38 40 95.0% -->
Totals Coverage Status
Change from base Build 9117465489: 0.3%
Covered Lines: 2473
Relevant Lines: 2887

💛 - Coveralls
BalzaniEdoardo commented 1 month ago

I re-based this into the dev branch, but up to @gviejo.

@qian-chu this is definitely going to be in main but we usually have merge first into dev, so that we have some time to try out the new feature.

qian-chu commented 1 month ago

Many thanks @BalzaniEdoardo for all the suggestions. I just moved the examples from merge_group() to merge(). Feel free to take a look. Not sure how to implement See Also merge_group() as a link but I did leave a mentioning there :)

I have no problem with dev vs main! Take your time.

BalzaniEdoardo commented 1 month ago

Many thanks @BalzaniEdoardo for all the suggestions. I just moved the examples from merge_group() to merge(). Feel free to take a look. Not sure how to implement See Also merge_group() as a link but I did leave a mentioning there :)

I have no problem with dev vs main! Take your time.

Great, I'll add the link, no worries!

codecov[bot] commented 1 month ago

Welcome to Codecov :tada:

Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests.

:information_source: You can also turn on project coverage checks and project coverage reporting on Pull Request comment

Thanks for integrating Codecov - We've got you covered :open_umbrella:

gviejo commented 1 month ago

Great PR. Thank you @qian-chu