[ENH] Add TsGroup.merge_group method #275
Conversation
There was a problem hiding this comment.
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:
- Run linters and formatters
black pynapple/core/ts_group.py
isort --check pynapple --profile black
flake8 pynapple --max-complexity 10
- Add tests of your method hitting all the exceptions. you can use
@pytest.mark.parametrizeto loop over multiple tsgroup and kwargs configs.
Cheers,
Edoardo
pynapple/core/ts_group.py
Outdated
| def merge(self, *tsgroups, reset_index=False, reset_time_support=False, ignore_metadata=False): | ||
| """ | ||
| Merge the TsGroup object with other TsGroup objects | ||
| See `TsGroup.merge_group` for more details |
There was a problem hiding this comment.
since this is probably the most user facing method, I would have a nice numpydoc docstring here too!
There was a problem hiding this comment.
That's a good suggestion. Could you offer some tips on how to do so while reducing redundancy?
There was a problem hiding this comment.
I suggest to move the detailed docstrings from the merge_group to the merge.
In merge_group keep the same initial paragraph describing the method, the parameters, and the returns sections. I'll add a See Also section that points to merge.
Co-authored-by: Edoardo Balzani <[email protected]>
Co-authored-by: Edoardo Balzani <[email protected]>
Co-authored-by: Edoardo Balzani <[email protected]>
Will do! Thanks for reviewing and providing feedback! |
pynapple/core/ts_group.py
Outdated
| def merge(self, *tsgroups, reset_index=False, reset_time_support=False, ignore_metadata=False): | ||
| """ | ||
| Merge the TsGroup object with other TsGroup objects | ||
| See `TsGroup.merge_group` for more details |
There was a problem hiding this comment.
I suggest to move the detailed docstrings from the merge_group to the merge.
In merge_group keep the same initial paragraph describing the method, the parameters, and the returns sections. I'll add a See Also section that points to merge.
tests/test_ts_group.py
Outdated
| nap.TsGroup.merge_group(ts_group, str, dict) | ||
| assert str(e_info.value) == f"Input at positions {[2, 3]} are not TsGroup!" |
There was a problem hiding this comment.
| with pytest.raises(TypeError) as e_info: | |
| nap.TsGroup.merge_group(ts_group, str, dict) | |
| assert str(e_info.value) == f"Input at positions {[2, 3]} are not TsGroup!" | |
| with pytest.raises(TypeError, match=f"Input at positions {[2, 3]} are not TsGroup!"): | |
| nap.TsGroup.merge_group(ts_group, str, dict) |
There was a problem hiding this comment.
Interestingly, this change gave a perplexing error
E AssertionError: Regex pattern did not match.
E Regex: 'Input at positions [2, 3] are not TsGroup!'
E Input: 'Input at positions [2, 3] are not TsGroup!'
E Did you mean to `re.escape()` the regex?
There was a problem hiding this comment.
Fixed with replacing {[2, 3]} with (.*)
Co-authored-by: Edoardo Balzani <[email protected]>
Pull Request Test Coverage Report for Build 9151076635Details
💛 - Coveralls |
Co-authored-by: Edoardo Balzani <[email protected]>
Co-authored-by: Edoardo Balzani <[email protected]>
|
Many thanks @BalzaniEdoardo for all the suggestions. I just moved the examples from I have no problem with |
Great, I'll add the link, no worries! |
Welcome to Codecov 🎉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. ℹ️ 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 ☂️ |
|
Great PR. Thank you @qian-chu |
Implements #251.
Added a static method
TsGroup.merge_group()as well as atsgroup.merge(). Documentation with examples is included as well.