-
-
Notifications
You must be signed in to change notification settings - Fork 173
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
Dof transform name updates #3149
Conversation
@@ -417,8 +425,7 @@ class FiniteElement | |||
const std::function<void(std::span<U>, std::span<const std::uint32_t>, | |||
std::int32_t, int)> | |||
sub_function | |||
= _sub_elements[0]->template dof_transformation_function<U>( | |||
ttype); | |||
= _sub_elements[0]->template dof_transformation_fn<U>(ttype); |
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.
@mscroggs This looks fishy (before and after the change). These lines are the only 'left-apply' functions in a function that is otherwise all 'right-apply'. What do you think it should be?
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.
Most likely this:
= _sub_elements[0]->template dof_transformation_fn<U>(ttype); | |
= _sub_elements[0]->template dof_transformation_right_fn<U>(ttype); |
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.
Hmm, with this change:
garth/transform-name-updates...garth/transform-test
tests fail:
https://github.com/FEniCS/dolfinx/actions/runs/8764139051
Any ideas?
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.
I'll take a closer look on Monday. I guess there's a good reason for not having the right that I need to re-work out and add a comment explaining
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.
There are some more transforms that look fishy and there be some cancellation by two incorrect transforms - I'll make a separate PR to highlight.
* API updates relating to: FEniCS/dolfinx#3149 * Updates due to: FEniCS/dolfinx#3119
Requires FEniCS/basix#807