Fix symmetric value shape #3535
Conversation
cpp/dolfinx/fem/FiniteElement.cpp
Outdated
| if (symmetric) // Consistency check for symmetric elements | ||
| { | ||
| if (!value_shape) | ||
| if (symmetric && value_shape){ |
There was a problem hiding this comment.
Fixed. I'll still suggest to wait so I test this in more elaborate tests in dolfiny. TBH, I just fixed the apparent places and I do not see completely through if all works downstream (the code is a bit complicated).
cpp/dolfinx/fem/FiniteElement.h
Outdated
| /// @param[in] symmetric Is the element a symmetric tensor? Should ony | ||
| /// set for 2nd-order tensor blocked elements. | ||
| FiniteElement(const basix::FiniteElement<geometry_type>& element, | ||
| std::optional<std::vector<std::size_t>> value_shape | ||
| = std::nullopt, | ||
| std::vector<std::size_t> value_shape = {}, int block_size = 1, |
There was a problem hiding this comment.
This was better as std::optional. It's confusing to have three inter-related arguments (element, value_shape and block_size) and the logic gets complicated. Simpler is (i) an element and (ii) optional value shape. It is also good to minimise the number of default arguments, especially for int and bool which can get mixed up. An if element is symmetric it gets even more confusing. We should avoid empty containers when we mean 'unset' rather than truly empty.
Doesn't mean that the member data for value_shape can't be a std::vector.
There was a problem hiding this comment.
The whole point of extra changes is to simplify this nontrivial code.
- My motivation for having block size as an extra argument is that the logic to compute it already lives in basix UFL wrapper, so it is better to avoid duplicating this nontrivial assumption, pass the one from basix instead. It does not have to have a default value.
I do not see the point of usingOkay, this is a bit confusing, especially the interaction withstd::optional<std::vector<>>. Optional is okay for when its value might not be provided. But empty std::vector is semantically correct and nicely maps in Python to an empty list or empty array. For lists/tuples on the Python side one does not useNoneto indicate an empty tuple/list. So here I strongly disagree.reference_value_shapeand vector/tensor valued basis.
I'll revert the changes to previous commit so the regression is fixed.
There was a problem hiding this comment.
This is reverted and should be good for review @garth-wells . Thanks for the comments. I had to fix the Quadrature element constructor as well as there could be a "blocked symmetric quadrature element" and none of dolfinx tests capture this.
garth-wells
added
bug
* Try with basix default value shape * Try block size from symmetric value shape * Fix block size logic * Point to basix branch * Fix oneAPI branch * Use basix branch in more workflows * Use basix branch in more workflows * Apply clang-format * Unify bs logic, fix for quadrature elements * Simplify value_shape * Revert "Simplify value_shape" This reverts commit f09414a. * Remove brew unlink * Move some code to the initialiser list * Revert CI to Basix main --------- Co-authored-by: Garth N. Wells <[email protected]>
Resolves #3516.
This PR makes symmetric rank-2 elements have
block size == dim * (dim + 1) / 2but keepsvalue_shape == (dim, dim).