-
Notifications
You must be signed in to change notification settings - Fork 159
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
Slate: Transpose(Tensor(form)) -> Tensor(adjoint(form)) #2272
Conversation
Looks like there's an actual failure in the hybridisation notebook? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Other than the actual test failure in the build, I think this is good.
Ok I think some tests were failing, because I took the ufl adjoint and not the one from |
Now it looks like we have many fails because |
Mhh, maybe this is not the right way to do it after all. Maybe I need to do it at a lower level. The leftover issues we have are that a) |
I think the difference is that:
Turns the argument with count 0 into the one with count 1, and vice versa whereas
just reverses the argument list. b) e.g. that This one should be the case, unless I misunderstand? |
Sorry I think I explained b) wrong actually. In the Slate optimiser we drop double transposes so that M.T.T is just M. If we introduce the change on Slate level we won't know when we have double transposes on terminal tensors anymore, so we can't do that optimisation. But in fact actually we don't need this optimisation on terminal tensors anymore if we go over the adjoint, I think, because we just do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you want to squash everything nicely, or shall I just do it in the merge commit? Thanks!
7822a43
to
3dc3e7d
Compare
…spose(Tensor(form) -> Tensor(adjoint(form)). This optimisation is introduced at the Slate optimiser level but essentially results in inlining transposes on tensors into the local assembly kernels generated by TSFC
3dc3e7d
to
ab71183
Compare
Sorry I had to force-push twice because I screwed up the squashing the first way round. If you give me an approving review, I will merge tomorrow myself as soon as the tests are passing. :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
I turned on auto-merge, so it might be done by the time you get up! |
This PR addresses #2262. It's a small codegen improvement which inlines transposes into the local assembly kernels generated by TSFC. I have done this on the highest level (in Slate) via
def __new__
.