search

firedrakeproject / tsfc

Two-stage form compiler
Other
15 stars 24 forks source link

More magic methods for gem Nodes #229

Closed wence- closed 4 years ago

wence- commented 4 years ago

Adds a __matmul__ method, and a componentwise helper so the existing sugar also works transparently for shaped objects.

Motivated by the gem->numpy->gem dance in FInAT/FInAT#63

miklos1 commented 4 years ago

I think this is good and well motivated by the example of the FInAT PR.

rckirby commented 4 years ago

These are welcome changes. However, If you see my comments on the FInAT PR, some of the gem <--> numpy dance is motivated by needing to build a V matrix/tensor by setting specific entries or blocks in a sparse matrix. Hence I start with a numpy array of zero objects and convert it (at the end) to gem. I need to be able set items/slices of ComponentTensor to do something like V[0:3, 0:3] = J.T @ J (not the right math, but gives a sample of something like what is needed. Otherwise, I may still be stuck with some gem <--> numpy business.

wence- commented 4 years ago

If the rvalue is a gem expression but the lvalue is an array, I think that's ok. In pure gem there is wrapping in a listtensor to achieve this.

There were just a bunch of places where it looked unnecessary, And potentially would create more temporaries in the final code.

rckirby commented 4 years ago

How to proceed? We could: 1.) Land this PR on tsfc and then change FInAT mappings to use it before we merge or 2.) Go ahead and merge the FInAT PR and after this tsfc one lands, go back and systematically change all the zany mappings (even the existing ones).

On Tue, Aug 25, 2020 at 5:37 PM Lawrence Mitchell notifications@github.com wrote:

If the rvalue is a gem expression but the lvalue is an array, I think that's ok. In pure gem there is wrapping in a listtensor to achieve this.

There were just a bunch of places where it looked unnecessary, And potentially would create more temporaries in the final code.

wence- commented 4 years ago

Maybe we should do the necessary to merge the finat PR "as is", then confirm that we can port everything we need to into just gem where possible on this branch (and make sure things work). And go round and fix all the zany elements. (So option 2).

rckirby commented 4 years ago

OK, I'll go back over comments on the FInAT PR and address the outstanding things not related to gem updates so we can merge.

On Wed, Aug 26, 2020 at 8:51 AM Lawrence Mitchell notifications@github.com wrote:

Maybe we should do the necessary to merge the finat PR "as is", then confirm that we can port everything we need to into just gem where possible on this branch (and make sure things work). And go round and fix all the zany elements. (So option 2).