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

[bug] [refactor] Fix error when ti.init() not called by deprecating Expr.layout_materialized #1347

Merged

Conversation

archibate
Copy link
Collaborator

Related issue = close #1335

[Click here for the format server]


I'm not sure why @yuanming-hu use Expr.layout_materialized instead of simply ti.get_runtime().materialized at the first place, maybe it was due to get_runtime() not done at that moment?

Copy link
Member

@yuanming-hu yuanming-hu left a comment

Choose a reason for hiding this comment

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

I'm not sure why @yuanming-hu use Expr.layout_materialized instead of simply ti.get_runtime().materialized at the first place, maybe it was due to get_runtime() not done at that moment?

Sorry, it's been too long and I simply don't remember :-)

Your implementation looks good. Do we still support @ti.layout after this PR?

@@ -114,7 +114,7 @@ def fill(self, val):
from .meta import fill_tensor
fill_tensor(self, val)

@deprecated('tensor.parent()', 'tensor.snode().parent()')
#@deprecated('tensor.parent()', 'tensor.snode().parent()')
Copy link
Member

Choose a reason for hiding this comment

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

So we no longer deprecate this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, since tensor.parent() returns a tensor while tensor.snode().parent() return a snode, it seems this is irreplaceable and many test is using this..



# The first test to run, ever:
def test_000_without_init():
Copy link
Member

Choose a reason for hiding this comment

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

What does 000 mean here and in the file name?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Seems pytest will starts from the lowest character order, so I use 000 to make test_000_without_init starts at the very first time when ti.init() has never been called in @ti.all_archs, to test if Taichi is functional even without ti.init().

Copy link
Member

Choose a reason for hiding this comment

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

Interesting - maybe we should simply let ti.reset clear everything, as if no ti.init() has been called? This will make the system more testable.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ti.reset()

I want to test if non of ti.reset or ti.init is called is working.
So, I'm creating a sandbox Taichi by creating a taichi_temp_test.py for test.

Btw, we may add a ti.reset() to taichi startup script to make your test method work? Not sure if this can crash doc-bots & make startup slower.

python/taichi/lang/impl.py Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Jul 1, 2020

Codecov Report

Merging #1347 into master will increase coverage by 0.03%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1347      +/-   ##
==========================================
+ Coverage   85.58%   85.61%   +0.03%     
==========================================
  Files          19       19              
  Lines        3371     3365       -6     
  Branches      624      624              
==========================================
- Hits         2885     2881       -4     
+ Misses        356      355       -1     
+ Partials      130      129       -1     
Impacted Files Coverage Δ
python/taichi/lang/expr.py 89.75% <100.00%> (-0.07%) ⬇️
python/taichi/lang/impl.py 89.93% <100.00%> (-0.17%) ⬇️
python/taichi/lang/snode.py 87.50% <100.00%> (ø)
python/taichi/lang/shell.py 44.06% <0.00%> (+3.38%) ⬆️

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 3c633f7...01b0fa2. Read the comment docs.

tests/python/test_runtime.py Outdated Show resolved Hide resolved
@archibate archibate requested a review from k-ye July 3, 2020 14:28
@archibate archibate added the LGTM label Jul 3, 2020
@archibate archibate requested a review from yuanming-hu July 4, 2020 02:01
@archibate archibate merged commit 4e37b9a into taichi-dev:master Jul 4, 2020
@FantasyVR FantasyVR mentioned this pull request Jul 4, 2020
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.

type object 'Expr' has no attribute 'layout_materialized'
4 participants