[lang] [test] Improve code coverage in SNode #1214

archibate · 2020-06-10T17:20:58Z

Related issue = #... (if any)

Thanks to Codecov, I found some of the Matrix functions like one and var wasn't functional at all..

Sleepy now, ff2mtoyo :)

rexwangcc

👍 for ⬆️ code coverage! I left my 2 cents.

python/taichi/lang/expr.py

tests/python/test_linalg.py

codecov · 2020-06-11T05:57:56Z

Codecov Report

Merging #1214 into master will decrease coverage by 0.28%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #1214      +/-   ##
==========================================
- Coverage   85.60%   85.31%   -0.29%     
==========================================
  Files          18       18              
  Lines        3285     3297      +12     
  Branches      621      614       -7     
==========================================
+ Hits         2812     2813       +1     
- Misses        345      349       +4     
- Partials      128      135       +7

Impacted Files	Coverage Δ
python/taichi/lang/__init__.py	`73.52% <100.00%> (+0.39%)`	⬆️
python/taichi/lang/expr.py	`89.75% <100.00%> (+1.69%)`	⬆️
python/taichi/lang/impl.py	`90.35% <100.00%> (+0.15%)`	⬆️
python/taichi/lang/matrix.py	`85.35% <100.00%> (-4.82%)`	⬇️
python/taichi/lang/snode.py	`86.81% <100.00%> (+9.47%)`	⬆️
python/taichi/lang/common_ops.py	`90.60% <0.00%> (-3.26%)`	⬇️
python/taichi/lang/ops.py	`93.65% <0.00%> (+0.82%)`	⬆️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1bebc23...d0bb76f. Read the comment docs.

archibate · 2020-06-11T07:12:24Z

python/taichi/lang/impl.py

+    def try_materialize(self):
+        if not self.layout_materialized:
+            self.materialize_layout_callback()


@yuanming-hu What's the difference between directly self.materialize which has a if not guard too, isn't it? What's the point of Expr.layout_materialized at the first place?

Why not response me so long :(

I believe he's extremely busy these days, especially last week, since he mentioned he had a paper deadline..

Sorry about the delayed reply. @archibate could you clarify your question? I'm not sure if I get you correctly.

Why you use a separate variable Expr.layout_materialized instead of simple use the existing ti.get_runtime().materialized? What the point of this design at first place?

I see - good point. I don't remember why I did that TBH. Maybe we should try to remove Expr.layout_materilized and use ti.get_runtime().materialized instead, and see if anything goes wrong (of course, in a future PR).

OK, I will. So far ti.get_runtime().layout_materialized is good enough in this PR, right? Or you want to revert that now?

k-ye

My feeling as a reviewer is that this PR once again tries to do too much. Initially I thought we were just going to fix some Matrix helpers that were not covered by tests. But then I found out it also touched materialize_layout and parent. Note that materialize_layout is a critical part of the system that requires careful reviews. Mixing it in will inevitably result in slow reviews...

I've left some comments, but I don't expect that will be enough to get this PR approved. I'd strongly suggest you not to touch the layout materialization part in this PR..

docs/syntax.rst

k-ye · 2020-06-16T10:57:40Z

python/taichi/lang/impl.py

+    def try_materialize(self):
+        if not self.layout_materialized:
+            self.materialize_layout_callback()


I believe he's extremely busy these days, especially last week, since he mentioned he had a paper deadline..

tests/python/test_tensor_reflection.py

k-ye · 2020-06-16T11:05:41Z

python/taichi/lang/snode.py

+        p = self.ptr
+        for i in range(n):
+            p = p.parent
+        if p.type == impl.taichi_lang_core.SNodeType.root:


IIRC, the parent of root is nullptr, so I think we should return None instead of making a cycle?

Co-authored-by: Ye Kuang <[email protected]>

archibate · 2020-06-18T03:24:33Z

python/taichi/lang/impl.py

-        Expr.materialize_layout_callback = self.materialize
+        self.materialize_layout_callback = self.materialize
+        self.layout_materialized = False


IIUC, Expr.layout_materialized is always equal to self.materialized?

Brain fried by noise, TTYL.

This reverts commit 00a05b2.

archibate · 2020-06-18T15:48:15Z

Hi, I moved the Matrix part to #1275, hope this helps reduce your review work :)

fix

archibate · 2020-06-22T06:13:58Z

Hello? Do you think this PR needs further break down? IMHO it's ready for review now.

yuanming-hu

Sorry about my delayed review. I recently got too many things to do. This PR is actually pretty well-written. Thanks! Please address a few minor concerns.

yuanming-hu · 2020-06-23T15:17:43Z

docs/snode.rst


-    Equivalent to ``len(tensor.shape())``.
+    Equivalent to ``snode.shape()[i]``, used in Taichi-scope to prevent `[...]` being transformed to tensor access.


IIRC snode.shape gives you a tuple and using [] on that doesn't get transformed to tensor access?

Seems we treated specially in ti.subscript? Cool!

Before preprocessing: @ti.kernel def func(): print(x.snode().shape()[1]) After preprocessing: def func(): ti.ti_print(ti.subscript(x.snode().shape(), 1))

So can we deprecate get_shape(i) and use shape()[i] now according to the python exactly-one-way-to-do-one-thing rule?

Sounds good. Maybe we should directly use shape as a @property, so that users can simply do a.shape instead of a.shape().

In this way, we can deprecate dim() as well, since that's simply len(a.shape). (I think users tend to think dim is the dimensionality of vectors/matrices instead of tensors.)

Can't agree more, many users are confusing dim with vector dimensions. Things we could do in another PR:

deprecate dim().

make shape() a @property.

document mat.n, mat.m in meta.rst.

tests/python/test_tensor_reflection.py

yuanming-hu

LGTM. Thanks!

archibate · 2020-06-23T16:53:39Z

Thank for approve, the 13 day PR finally draw it's end...

* [lang] [test] Improve code coverage in Matrix and fix some methods * [skip ci] enforce code format * Improve tensor_reflection test * Better SNode with __repr__ * [skip ci] apply * [skip ci] Apply suggestions from code review Co-authored-by: Ye Kuang <[email protected]> * revert k-ye * Revert {Expr => PyTaichi}.materalized_layout_callback * Revert "[lang] [test] Improve code coverage in Matrix" This reverts commit 00a05b2. * [skip ci] Fix Matrix.snode() not found fix * [skip ci] loop_range().snode() * improve & deprecate duplicated functions * [skip ci] enforce code format Co-authored-by: Taichi Gardener <[email protected]> Co-authored-by: Ye Kuang <[email protected]>

[lang] [test] Improve code coverage in Matrix and fix some methods

e18b633

archibate requested review from yuanming-hu, rexwangcc, KLozes and taichi-gardener June 10, 2020 17:20

[skip ci] enforce code format

5fa8989

rexwangcc reviewed Jun 11, 2020

View reviewed changes

python/taichi/lang/expr.py Show resolved Hide resolved

tests/python/test_linalg.py Outdated Show resolved Hide resolved

archibate added the skip ci label Jun 11, 2020

Improve tensor_reflection test

34a775a

archibate requested a review from rexwangcc June 11, 2020 06:05

Better SNode with __repr__

71452f0

archibate changed the title ~~[lang] [test] Improve code coverage in Matrix and fix some methods~~ [lang] [test] Improve code coverage in Matrix and SNode Jun 11, 2020

archibate commented Jun 11, 2020

View reviewed changes

archibate added the large changeset The PR contains a large changeset and reviewing may take times label Jun 11, 2020

archibate requested review from rexwangcc and removed request for rexwangcc June 11, 2020 15:51

archibate removed the skip ci label Jun 14, 2020

archibate requested a review from k-ye June 16, 2020 02:50

k-ye reviewed Jun 16, 2020

View reviewed changes

archibate and others added 2 commits June 16, 2020 22:49

[skip ci] apply

c118b96

[skip ci] Apply suggestions from code review

2170995

Co-authored-by: Ye Kuang <[email protected]>

archibate requested review from k-ye and removed request for k-ye June 17, 2020 12:30

Merge branch 'master' into impcovmat

b29dab0

archibate commented Jun 18, 2020

View reviewed changes

archibate added 2 commits June 18, 2020 12:41

revert k-ye

0cc5d4d

Revert {Expr => PyTaichi}.materalized_layout_callback

66b169f

archibate changed the title ~~[lang] [test] Improve code coverage in Matrix and SNode~~ [lang] [test] Improve code coverage in SNode Jun 18, 2020

archibate mentioned this pull request Jun 18, 2020

[lang] [test] Improve code coverage in Matrix #1275

Merged

Revert "[lang] [test] Improve code coverage in Matrix"

c639431

This reverts commit 00a05b2.

archibate requested a review from k-ye June 18, 2020 16:09

[skip ci] Fix Matrix.snode() not found

6ca0194

fix

archibate force-pushed the impcovmat branch from 4d3a5c1 to 6ca0194 Compare June 18, 2020 16:13

[skip ci] loop_range().snode()

9ca1d67

archibate assigned yuanming-hu Jun 23, 2020

yuanming-hu reviewed Jun 23, 2020

View reviewed changes

archibate and others added 3 commits June 23, 2020 23:42

[skip ci] Merge branch 'master' into impcovmat

6d37a8b

improve & deprecate duplicated functions

3431c47

[skip ci] enforce code format

d0bb76f

archibate requested a review from yuanming-hu June 23, 2020 16:49

yuanming-hu approved these changes Jun 23, 2020

View reviewed changes

archibate added the LGTM label Jun 23, 2020

archibate merged commit 307b676 into taichi-dev:master Jun 23, 2020

FantasyVR mentioned this pull request Jun 24, 2020

[release] v0.6.13 #1317

Merged

archibate mentioned this pull request Jun 24, 2020

[Lang] [refactor] Deprecate x.shape() and x.dim(), use x.shape instead #1318

Merged

3 tasks

k-ye mentioned this pull request Jun 24, 2020

[lang] Return None when parent() exceeds its maximum depth #1319

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[lang] [test] Improve code coverage in SNode #1214

[lang] [test] Improve code coverage in SNode #1214

archibate commented Jun 10, 2020 •

edited

Loading

rexwangcc left a comment

codecov bot commented Jun 11, 2020 •

edited

Loading

archibate Jun 11, 2020

archibate Jun 15, 2020

k-ye Jun 16, 2020

yuanming-hu Jun 18, 2020

archibate Jun 18, 2020

yuanming-hu Jun 18, 2020

archibate Jun 18, 2020

k-ye left a comment •

edited

Loading

k-ye Jun 16, 2020

k-ye Jun 16, 2020

archibate Jun 18, 2020

archibate Jun 18, 2020

archibate commented Jun 18, 2020

archibate commented Jun 22, 2020

yuanming-hu left a comment

yuanming-hu Jun 23, 2020

archibate Jun 23, 2020

archibate Jun 23, 2020

yuanming-hu Jun 23, 2020

archibate Jun 23, 2020

yuanming-hu left a comment

archibate commented Jun 23, 2020


		Equivalent to ``len(tensor.shape())``.
		Equivalent to ``snode.shape()[i]``, used in Taichi-scope to prevent `[...]` being transformed to tensor access.

[lang] [test] Improve code coverage in SNode #1214

[lang] [test] Improve code coverage in SNode #1214

Conversation

archibate commented Jun 10, 2020 • edited Loading

rexwangcc left a comment

Choose a reason for hiding this comment

codecov bot commented Jun 11, 2020 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k-ye left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

archibate commented Jun 18, 2020

archibate commented Jun 22, 2020

yuanming-hu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yuanming-hu left a comment

Choose a reason for hiding this comment

archibate commented Jun 23, 2020

archibate commented Jun 10, 2020 •

edited

Loading

codecov bot commented Jun 11, 2020 •

edited

Loading

k-ye left a comment •

edited

Loading