[RELAY][IR] Move type_annotation to Var, remove Param #1900

tqchen · 2018-10-14T22:30:58Z

Move type_annotation as a field of Var
- Remove value_type in Let
- Remove Param and use Var instead
Add/correct docstring of all python Expr class, make them self-contained.

The reasoning behind the change:

The current Param was a bit confusing and is only used in the place of the function parameter declaration. Folding the type annotation into Var allows removing one concept.
This will make the future graph style programming easier, which I will follow up with subsequent PR.

tqchen · 2018-10-14T22:31:25Z

@MarisaKirisame @jroesch @slyubomirsky @joshpoll please review

joshpoll · 2018-10-14T22:33:31Z

This actually makes a lot of sense and simplifies several things on the front end. 👍

MarisaKirisame · 2018-10-14T22:38:16Z

I just did a skim, and LGTM.
Should we rename typeparam -> typevar then?

tqchen · 2018-10-14T22:45:20Z

We can discuss typeparam vs typevar later with @jroesch

joshpoll · 2018-10-14T22:37:51Z

include/tvm/relay/expr.h

-  /*! \brief The tuple */
+  /*! \brief The tuple expression */
  Expr tuple;
  /*! \brief which value to get */


joshpoll · 2018-10-14T22:38:47Z

python/tvm/relay/expr.py

    @property
    def checked_type(self):
-        """Get the checked type of relay.
+        """Get the checked type of tvm.relay.


Should this be "...of a tvm.relay type." ?

joshpoll · 2018-10-14T22:39:21Z

python/tvm/relay/expr.py

 class Constant(Expr):
-    """A constant tensor in Relay, see tvm/relay/type.h for more details.
-    """
+    """A constant expression in Tvm.Relay.


"... in tvm.relay."

joshpoll · 2018-10-14T22:40:10Z

python/tvm/relay/expr.py


+    Parameters
+    ----------
+    fields : list of tvm.relay.Expr.


"fields: List[tvm.relay.Expr]"

joshpoll · 2018-10-14T22:40:22Z

python/tvm/relay/expr.py

 @register_relay_node
 class Var(Expr):
-    """A local variable in Relay."""
+    """A local variable in Tvm.Relay.


"tvm.relay."

joshpoll · 2018-10-14T22:47:57Z

python/tvm/relay/expr.py

+    attrs: tvm.Attrs, optional
+        Attributes to the call, can be None
+
+    type_args: list of tvm.relay.Type


type_args: List[tvm.relay.Type]

joshpoll · 2018-10-14T22:48:12Z

python/tvm/relay/expr.py

+
+    type_args: list of tvm.relay.Type
+        The additional type arguments, this is only
+        used in advanced usecase of template functions.


same edit as previous.

joshpoll · 2018-10-14T22:51:43Z

python/tvm/relay/expr.py

+    Parameters
+    ----------
+    var: tvm.relay.Var
+        The local variable to be binded.


binded -> bound

joshpoll · 2018-10-14T22:51:49Z

python/tvm/relay/expr.py

+        The local variable to be binded.
+
+    value: tvm.relay.Expr
+        The value to be binded.


binded -> bound

joshpoll · 2018-10-14T22:52:20Z

python/tvm/relay/expr.py

 @register_relay_node
 class If(Expr):
-    """A conditional expression in Relay, see tvm/relay/expr.h for more details."""
+    """A conditional expression in Relay.


Relay -> tvm.relay

MarisaKirisame · 2018-10-14T22:49:12Z

python/tvm/relay/expr.py

    @property
    def checked_type(self):
-        """Get the checked type of relay.
+        """Get the checked type of tvm.relay.


of relay expr?

MarisaKirisame · 2018-10-14T22:51:59Z

python/tvm/relay/expr.py

 @register_relay_node
 class Call(Expr):
-    """A function call in Relay, see tvm/relay/expr.h for more details."""
+    """Function call node in Relay.


MarisaKirisame · 2018-10-14T22:52:50Z

src/relay/ir/debug_printer.cc

    std::vector<Doc> vec;
-    for (size_t i = 0; i < arr.size(); ++i) {
-      vec.push_back(Docify(arr[i]));
+    for (Var param : arr) {


can you call docify directly?

My understanding is that we cannot directly docify because there are two behaviors(at the declaration point where the type annotation need to be printed), at the usage point where we don't want to print type annotations

tqchen · 2018-10-14T23:21:54Z

@MarisaKirisame @joshpoll Thanks for the reviews, I have made changes according to your comments

slyubomirsky · 2018-10-15T00:19:25Z

Agreed with previous comments, this is certainly a good simplification to make. LGTM, for the most part.

slyubomirsky · 2018-10-15T00:28:06Z

tests/python/relay/test_pass_alpha_equal.py

    test_tensor_type_alpha_equal()
    test_incomplete_type_alpha_equal()
    test_constant_alpha_equal()
-    test_type_param_alpha_equal()


Why have test_type_param_alpha_equal() and test_var_alpha_equal() been eliminated from the main method? Those test cases don't appear to have been eliminated.

was removed because TypeParam nolonger exists

tqchen · 2018-10-15T01:09:04Z

Thanks @slyubomirsky @joshpoll @MarisaKirisame , this is merged

[RELAY][IR] Move type_annotation to Var, remove Param

9fc94ae

joshpoll suggested changes Oct 14, 2018

View reviewed changes

MarisaKirisame reviewed Oct 14, 2018

View reviewed changes

Address review comments

a4d08fe

slyubomirsky reviewed Oct 15, 2018

View reviewed changes

tqchen merged commit 0b4cc05 into apache:master Oct 15, 2018

slyubomirsky mentioned this pull request Oct 15, 2018

[Relay] Fix IncompleteType Alpha Equality Checking #1901

Closed

FrozenGene pushed a commit to FrozenGene/tvm that referenced this pull request Dec 27, 2018

[RELAY][IR] Move type_annotation to Var, remove Param (apache#1900)

df27969

[RELAY][IR] Move type_annotation to Var, remove Param #1900

[RELAY][IR] Move type_annotation to Var, remove Param #1900

Uh oh!

Conversation

tqchen commented Oct 14, 2018

Uh oh!

tqchen commented Oct 14, 2018

Uh oh!

joshpoll commented Oct 14, 2018

Uh oh!

MarisaKirisame commented Oct 14, 2018

Uh oh!

tqchen commented Oct 14, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tqchen commented Oct 14, 2018

Uh oh!

slyubomirsky commented Oct 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tqchen commented Oct 15, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

slyubomirsky commented Oct 15, 2018 •

edited

Loading