-
Notifications
You must be signed in to change notification settings - Fork 70
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
Enhance mask_indices and move_axis #622
Enhance mask_indices and move_axis #622
Conversation
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.
These changes are very nice, only a few small comments
KS = [0, -1, 1, -2, 2] | ||
FUNCTIONS = ["tril", "triu"] |
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.
prefer tuples unless mutability is an explicit need
KS = [0, -1, 1, -2, 2] | |
FUNCTIONS = ["tril", "triu"] | |
KS = (0, -1, 1, -2, 2) | |
FUNCTIONS = ("tril", "triu") |
(here, and also several places below)
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.
You are right. Tuple is better than list if the case is immutable. Fixed.
tests/integration/test_moveaxis.py
Outdated
EMPTY_ARRAYS = [ | ||
[], | ||
[[]], | ||
[[], []], | ||
] |
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.
EMPTY_ARRAYS = [ | |
[], | |
[[]], | |
[[], []], | |
] | |
EMPTY_ARRAYS = ( | |
[], | |
[[]], | |
[[], []], | |
) |
|
||
msg = "'str' object is not callable" | ||
with pytest.raises(TypeError, match=msg): | ||
num.mask_indices(10, "abc") |
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.
This would be better split into two separate named tests
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.
Fixed.
cn.moveaxis(self.x, None, 0) | ||
|
||
with pytest.raises(TypeError, match=msg): | ||
cn.moveaxis(self.x, 0, None) |
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.
split into separate tests for float and None
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.
Fixed
No description provided.