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] Fix matrix operations to use numpy only in Python scope #1208

Merged
merged 2 commits into from
Jun 10, 2020

Conversation

k-ye
Copy link
Member

@k-ye k-ye commented Jun 10, 2020

is_pyconstant is not enough because there are cases where constant ti.Vectors are used in the taichi scope. For example:

light_normal = ti.Vector([0.0, -1.0, 0.0])

This PR adds in_taichi_scope() and in_python_scope() helpers to make cornell box run again. As a slightly off the topic change, I also adopted the new gui.is_running to this example.

Related issue = #1051, #1190

@k-ye k-ye requested review from archibate and rexwangcc June 10, 2020 13:51
@rexwangcc
Copy link
Member

Thanks for the fix @k-ye ! AFIK, there are 3 efforts now trying to resolve this issue: #1184 #1188 , since we are trying to fix it in the next release asap, we might want to come up with a plan on how to consolidate them and what is the short/long term plan for this.

Besides that, in_python_scope is better than what's used in #1184 !

@codecov
Copy link

codecov bot commented Jun 10, 2020

Codecov Report

Merging #1208 into master will increase coverage by 0.22%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1208      +/-   ##
==========================================
+ Coverage   66.85%   67.08%   +0.22%     
==========================================
  Files          35       35              
  Lines        4827     4827              
  Branches      885      885              
==========================================
+ Hits         3227     3238      +11     
+ Misses       1424     1414      -10     
+ Partials      176      175       -1     
Impacted Files Coverage Δ
python/taichi/lang/matrix.py 84.79% <0.00%> (+1.85%) ⬆️

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 7562943...eb2578d. Read the comment docs.

@archibate
Copy link
Collaborator

Thank for fixing this! We may merge this for a temp solution in v0.6.9. But IMHO for long term consideration, v0.6.10, we should use #1188 as ultimate solution, which doesn't rely on to_numpy.

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.

LGTM! Thanks!

@k-ye
Copy link
Member Author

k-ye commented Jun 10, 2020

Thanks for the quick review, and sorry about the redundant works! I agree that we should do this systematically.

@k-ye k-ye merged commit bec9bdb into taichi-dev:master Jun 10, 2020
@k-ye k-ye deleted the mat branch June 10, 2020 14:42
Copy link
Member

@rexwangcc rexwangcc left a comment

Choose a reason for hiding this comment

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

LGTM! This PR now supersedes #1184

@archibate
Copy link
Collaborator

Thank for the quick solution, please don't forget to take a look at #1188 when you have time :)

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.

None yet

4 participants