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.

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

Ultima supports constructor (partially) #121

Merged
merged 7 commits into from
Sep 25, 2018

Conversation

vorj
Copy link

@vorj vorj commented Sep 13, 2018

Ultima in this PR supports member functions.
And the ultima also supports constructors partially.

@vorj vorj requested a review from LWisteria September 13, 2018 09:21
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 9b9fbc1) passed in vega.

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 9b9fbc1) passed in titanv.

@vorj
Copy link
Author

vorj commented Sep 13, 2018

@LWisteria please review it.

I found many bugs...

@vorj vorj assigned vorj and unassigned LWisteria Sep 13, 2018
@vorj vorj changed the title Ultima supports member function WIP : Ultima supports member function Sep 13, 2018
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 c5dadd8) passed in titanv.

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 c5dadd8) passed in vega.

@vorj vorj changed the title WIP : Ultima supports member function Ultima supports member function Sep 14, 2018
@vorj vorj assigned LWisteria and unassigned vorj Sep 14, 2018
@vorj
Copy link
Author

vorj commented Sep 14, 2018

@LWisteria OK, it's ready to review.

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.

Please describe why this PR is needed and what this PR is fixing.

@LWisteria LWisteria assigned vorj and unassigned LWisteria Sep 18, 2018
@vorj
Copy link
Author

vorj commented Sep 18, 2018

@LWisteria
For now, we are trying to support clpy.core.fusion .
However, when we tried to use fused clpy.core.min (, max , argmin , argmax , nanmin , and nanmax ), current ClPy caused UltimaRuntimeError because current clpy.core.fuse depends on implicit conversion on generated C++ codes.
The functions uses struct min_max_st_T , which is just a C structure, but on original CuPy it was template<typename T> struct min_max_st<T> , which has (non-explicit) constructor from double value (it means that CuPy's min_max_st<T> can be constructed from double value implicitly ).

I think that the behavior of current fuse that it depends on implicit conversion is the exactly the problem.
However, current CuPy doesn't have the problematic codes, so we shouldn't fix the fuse implementation codes directly ( because the codes will be thrown away when we update CuPy base version(#33)).

In these reasons, I proposed partially constructor emulation on ultima.
The member function support is just additional implementation (it shares almost all codes with constructor implementation).

@vorj vorj assigned LWisteria and unassigned vorj Sep 20, 2018
@vorj vorj mentioned this pull request Sep 20, 2018
@LWisteria
Copy link
Member

The member function support is just additional implementation

YAGNI. Fewer functions are better to reduce bugs. Please remove it.

And you need test to check implicit conversion constructor.

@LWisteria LWisteria assigned vorj and unassigned LWisteria Sep 21, 2018
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 5de0af3) passed in vega.

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 5de0af3) passed in titanv.

@vorj vorj assigned LWisteria and unassigned vorj Sep 25, 2018
@vorj
Copy link
Author

vorj commented Sep 25, 2018

@LWisteria I fixed it. Please review it.

@LWisteria
Copy link
Member

Please fix PR title and descriptions

@vorj vorj changed the title Ultima supports member function Ultima supports constructor (partially) Sep 25, 2018
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 5b4720b) passed in titanv.

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 5b4720b) passed in vega.

@vorj
Copy link
Author

vorj commented Sep 25, 2018

@LWisteria I did it. Please review it.

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 3b6da91 into clpy Sep 25, 2018
@LWisteria LWisteria deleted the ultima-supports-member-function branch September 25, 2018 08:19
@vorj vorj mentioned this pull request Sep 25, 2018
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants