[Relay] add Tuple pattern #3596
Conversation
| @@ -18,7 +18,7 @@ | |||
| */ | |||
|
|
|||
| /*! | |||
| * Copyright (c) 2018 by Contributors | |||
| * Copyright (c) 2019 by Contributors | |||
There was a problem hiding this comment.
In my experience such changes are usually a year range. does the ASF not need a year range here ?
git blame suggests this file was created in 2018, thus this should read copyright 2018-19 IIUC. However IANAL and the exact definition depends on project policy.
@tqchen , any thoughts
There was a problem hiding this comment.
Thank you for catching this! I just wrongly assume to use the latest year whenever I update a file.
| @@ -18,8 +18,8 @@ | |||
| */ | |||
|
|
|||
| /*! | |||
| * Copyright (c) 2018 by Contributors | |||
| * \file src/tvm/relay/pattern_functor.cc | |||
| * Copyright (c) 2019 by Contributors | |||
There was a problem hiding this comment.
I think this is a typo - however @tqchen needs to comment as he was the last one who changed this. The first commit came in 2019, so you are safe here.
|
Very glad you added this; it will certainly be a convenience. I will give a detailed review later. |
|
High-level feedback that you probably guessed: We'll need to add test cases for the features where you added tuple patterns (type inference, the interpreter, match completeness checking, partial eval, etc.) |
|
@slyubomirsky I changed the prelude zip, so it work for ti/interpreter/vm/match completeness without any additional code. Partial Eval also PE all prelude function so at least the code run and typecheck. |
|
@slyubomirsky can you explicitly review cc @wweic |
|
Again, I think specific unit tests (for alpha-equality, match completeness, etc.), not just by changing the implementations of prelude functions (though that's handy too), would still be very important to have. I have read over the implementation of the feature and do not have any issues with it, but I stress the importance of having separate tests since if something goes wrong or a later feature interacts badly with what we have, specific tests will make it much easier to identify what went wrong. |
There was a problem hiding this comment.
lgtm. I agree with @slyubomirsky, it would be good to add specific test cases for features introduced in the PR. It's also good enhancement to documentation of the features, people can learn what's the minimal example of tuple pattern.
|
@MarisaKirisame and @slyubomirsky can you guys get this one over the finish line this week? |
MarisaKirisame
force-pushed
the
tuple-pattern
branch
from
9008e71
to
5e00081
Compare
|
@jroesch @slyubomirsky I had added the test. it is now good. |
|
@vinx13 @slyubomirsky @wweic @junrushao1994 can you guys give another round of review? |
|
Thanks for adding tests. LGTM |
* implement tuple pattern * add tuple pattern * lint; * lint * lint * fix error * fix * add test
* implement tuple pattern * add tuple pattern * lint; * lint * lint * fix error * fix * add test
* implement tuple pattern * add tuple pattern * lint; * lint * lint * fix error * fix * add test
* implement tuple pattern * add tuple pattern * lint; * lint * lint * fix error * fix * add test
Thanks for contributing to TVM! Please refer to guideline https://docs.tvm.ai/contribute/ for useful information and tips. After the pull request is submitted, please request code reviews from Reviewers.
@icemelon9 @slyubomirsky @Yulun-Yao @vinx13 can you guys help review?