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

[Lang] [refactor] Add Field classes for ti.field/ti.Vector.field/ti.Matrix.field #2638

Merged
merged 13 commits into from Aug 6, 2021

Conversation

strongoier
Copy link
Contributor

@strongoier strongoier commented Aug 5, 2021

Related issue = close #2499

Previously, a ti.field is a Expr with is_global()=True, while a ti.Matrix.field is a Matrix with is_global()=True. This is not intuitive. After this PR, ti.field will become a ScalarField, and ti.Matrix.field will become a MatrixField, which both inherit from a super class Field.

The Field class keeps the original implementation way of using SNodes. A field is constructed by a list of field members. For example, a scalar field has 1 field member, while a 3x3 matrix field has 9 field members. A field member is a Python Expr wrapping a C++ GlobalVariableExpression. A C++ GlobalVariableExpression wraps the corresponding SNode.

All methods belonging to ti.field/ti.Matrix.field have been migrated from Expr/Matrix classes to ScalarField/MatrixField classes. If a method is implemented exactly the same in both classes, it will be put into Field class. Otherwise common methods will be marked as NotImplemented in Field class to avoid misuse.

During migration, the property methods and meta (fill, to/from numpy/torch, copy) methods are mainly kept the same with slight cleaning in both implementation and docstrings. The biggest change is around subscript (both in Python scope and Taichi scope) - the main idea is that for MatrixField, after first subscript with coordinates, we should get a Matrix, where each element is under the same state as if we are accessing a corresponding ScalarField. It acts just like a Matrix so no more Proxy classes are needed. By the way, Matrix.with_entries() is just added for this purpose.

Note:

  • Previously, when x is a field with 0-D shape, although deprecated, it can still be accessed via x instead of x[None]. After this PR this invalid behavior will not work any more.
  • Deprecated interfaces of these two classes are removed.

@strongoier
Copy link
Contributor Author

/format

Copy link
Member

@k-ye k-ye left a comment

Choose a reason for hiding this comment

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

Awesome! Could you briefly write up what's been added/refactored to ease the review process? An excellent example is #2495 (which is not that "brief"...)

python/taichi/lang/field.py Outdated Show resolved Hide resolved
python/taichi/lang/field.py Outdated Show resolved Hide resolved
@strongoier
Copy link
Contributor Author

Awesome! Could you briefly write up what's been added/refactored to ease the review process? An excellent example is #2495 (which is not that "brief"...)

Updated.

Copy link
Member

@k-ye k-ye 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 for the huge efforts!

Copy link
Member

@victoriacity victoriacity 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 will make #2627 much clearer than what it is now. It will also facilitate the implementation of recursive custom structs.

@strongoier
Copy link
Contributor Author

/format

@victoriacity
Copy link
Member

P.S. It looks like taichi_glsl defines extensions of the current Matrix/Expr classes. I'm not sure if the package will still be fully functional after the changes are merged.

@strongoier strongoier merged commit efd66b5 into taichi-dev:master Aug 6, 2021
@k-ye
Copy link
Member

k-ye commented Aug 6, 2021

I'm not sure if the package will still be fully functional after the changes are merged.

Thanks for catching this! Let's get this in first to move other things forward :-)

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.

ti.field() returns a "field" class instead of taichi.lang.expr.Expr
4 participants