-
Notifications
You must be signed in to change notification settings - Fork 74
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix trim_zeros_frames to trim zeros from the end #87
Conversation
f77692c
to
13b4721
Compare
Codecov Report
@@ Coverage Diff @@
## master #87 +/- ##
==========================================
+ Coverage 83.85% 83.98% +0.13%
==========================================
Files 31 31
Lines 1697 1711 +14
==========================================
+ Hits 1423 1437 +14
Misses 274 274
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for raising the issue. To keep compatibility, could you make trim
an option to the trim_zeros_frames and defaults to 'fb'' and fix docstring accordingly?
def trim_zeros_frames(x, eps=1e-7, trim="fb"):
'''Remove leading and/or trailing zeros frames
...
Thank you for your review. 7874393 But still trimming issue remains...
These behaviors can be confirmed from these commits. Thanks. |
7c25d26
to
e8d181f
Compare
Could you put a change log in https://github.com/r9y9/nnmnkwii/blob/1649b3caba671ad203c4d599b5b8b95c168e03ac/docs/changelog.rst#v0019-2019-xx-xx? Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. I'll merge when CI succeeded.
Docs of trim_zeros_frames says
Remove trailing zeros frames
.But it behaves removing both front and back zero frames because of lack of
np.trim_zeros
'strim
option.expected
actual
So I thought that it's better to set option
trim='b'
.