Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Pass test_rearrange.py #133

Merged
merged 7 commits into from
Oct 2, 2018
Merged

Pass test_rearrange.py #133

merged 7 commits into from
Oct 2, 2018

Conversation

ghost
Copy link

@ghost ghost commented Oct 2, 2018

solve #91
add import flip, fliplr, flipud, roll, rot90 to clpy/init.py
change exception type for invalid axis
add test_rearrange.py to ci

@ghost ghost self-assigned this Oct 2, 2018
jenkins-maekawa
jenkins-maekawa approved these changes Oct 2, 2018
View reviewed changes
Copy link

@jenkins-maekawa jenkins-maekawa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test (commit 89c71ec) passed in vega.

jenkins-maekawa
jenkins-maekawa approved these changes Oct 2, 2018
View reviewed changes
Copy link

@jenkins-maekawa jenkins-maekawa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test (commit 89c71ec) passed in titanv.

jenkins-maekawa
jenkins-maekawa approved these changes Oct 2, 2018
View reviewed changes
Copy link

@jenkins-maekawa jenkins-maekawa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test (commit f4dfcf3) passed in vega.

jenkins-maekawa
jenkins-maekawa approved these changes Oct 2, 2018
View reviewed changes
Copy link

@jenkins-maekawa jenkins-maekawa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test (commit f4dfcf3) passed in titanv.

LWisteria
LWisteria requested changes Oct 2, 2018
View reviewed changes
clpy/manipulation/rearrange.py Outdated
@@ -20,11 +20,11 @@ def flip(a, axis):
"""
a_ndim = a.ndim
if a_ndim < 1:
raise ValueError('Input must be >= 1-d')
raise core.core._AxisError('Input must be >= 1-d')
Copy link
Member

@LWisteria LWisteria Oct 2, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do you modify this?

Does this cause any problem on test_rearrange.py?
If not, you should not make any changes now. This problem should be resolved when we update the base version and merge cupy/cupy@0d28378.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ValueError cause Error at 3 test cases

clpy/tests/clpy_tests/manipulation_tests/test_rearrange.py

Line 179 in e135d7c

def test_flip_insufficient_ndim(self, xp, dtype):

clpy/tests/clpy_tests/manipulation_tests/test_rearrange.py

Line 185 in e135d7c

def test_flip_invalid_axis(self, xp, dtype):

clpy/tests/clpy_tests/manipulation_tests/test_rearrange.py

Line 191 in e135d7c

def test_flip_invalid_negative_axis(self, xp, dtype):

So, I modify exception.
Comment outing these test case is better than this modification?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean that the original CuPy v2.1.0 gets same error if no modification?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes.
I test cupy v2.1.0 and get errors at same 3 test cases.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Eeeeeeee...?
We have to wait v5.0.0 to resolve this problem on CuPy...?

OK, leave that aside. Currently our target is CuPy v2.1.0. So current ClPy (v2.1.0x) should behave same as it even if it causes some errors.

Please revert the modification and modify test_rearrange.py. And make comments about this problem on the test codes and make a new issue to revert the modirication when #33 would be resolved.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I revert rearrange.py 's modification and modify test_rearrange.py.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I made new issue about this problem.

jenkins-maekawa
jenkins-maekawa approved these changes Oct 2, 2018
View reviewed changes
Copy link

@jenkins-maekawa jenkins-maekawa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test (commit 482bb8f) passed in vega.

jenkins-maekawa
jenkins-maekawa approved these changes Oct 2, 2018
View reviewed changes
Copy link

@jenkins-maekawa jenkins-maekawa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test (commit 482bb8f) passed in titanv.

jenkins-maekawa
jenkins-maekawa approved these changes Oct 2, 2018
View reviewed changes
Copy link

@jenkins-maekawa jenkins-maekawa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test (commit 240a00f) passed in titanv.

jenkins-maekawa
jenkins-maekawa approved these changes Oct 2, 2018
View reviewed changes
Copy link

@jenkins-maekawa jenkins-maekawa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test (commit 240a00f) passed in vega.

@ghost ghost mentioned this pull request Oct 2, 2018
Closed
@ghost
Copy link
Author

ghost commented Oct 2, 2018

@LWisteria Could you review it?

LWisteria
LWisteria approved these changes Oct 2, 2018
View reviewed changes
Copy link
Member

@LWisteria LWisteria left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@LWisteria LWisteria merged commit 8bea894 into clpy Oct 2, 2018
@LWisteria LWisteria deleted the 91-pass_test_rearrange branch October 2, 2018 06:50
@LWisteria LWisteria mentioned this pull request Oct 2, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@LWisteria @jenkins-maekawa @nstomos