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

Expose basix::FiniteElement #2205

Merged
merged 11 commits into from
Jun 6, 2022
Merged

Expose basix::FiniteElement #2205

merged 11 commits into from
Jun 6, 2022

Conversation

nate-sime
Copy link
Contributor

Exposure of the basix::FiniteElement underlying dolfinx::fem::FiniteElement is very useful for code building on top of dolfinx. Futhermore a number of methods in dolfinx::fem::FiniteElement needlessly wrap the basix::FiniteElement. Here we simplify the interface.

@nate-sime nate-sime changed the title Nate/expose basix element Expose basix::FiniteElement May 25, 2022
@garth-wells
Copy link
Member

The idea behind the original design was to hide the Basix implementation, for example to allow a user to hand-code an element. We've probably had some leakage over time, but is it still reasonable to try and hide the implementation?

A benefit of wrapping parts of the Basix interface is that we can more easily evolve Basix and DOLFINx independently. Some changes in Basix are planned to remove xtensor.

@nate-sime
Copy link
Contributor Author

It seems that most natural way to creating a custom element is to use basix itself as demonstrated here. I can add back the wrapping methods for those who wish to inherit dolfinx::fem::FiniteElement. Whichever philosophy is decided, having access to the underlying basix::FiniteElement is highly desirable.

@jorgensd
Copy link
Sponsor Member

It seems that most natural way to creating a custom element is to use basix itself as demonstrated here. I can add back the wrapping methods for those who wish to inherit dolfinx::fem::FiniteElement. Whichever philosophy is decided, having access to the underlying basix::FiniteElement is highly desirable.

I agree with Nate that custom elements now are naturally supported through Basix. We also added the function space constructor that takes in a Basix element, and then create the dolfinx finite element.

There are many cases where one would like to access functions we haven’t exposed, such as degree and tabulate_shape, that would simplify codes relying on DOLFINx in C++.

@IgorBaratta
Copy link
Member

I also think it's a good idea to expose the basix element, although the wrapping methods provide more freedom for Basix. Otherwise, a change in basix interface would reflect in dolfinx API.

@jorgensd
Copy link
Sponsor Member

I also think it's a good idea to expose the basix element, although the wrapping methods provide more freedom for Basix. Otherwise, a change in basix interface would reflect in dolfinx API.

We could keep the wrappers, but also expose the underlying element such that one can use the other functions (that are not used in the DOLFINx API).

@garth-wells
Copy link
Member

Yes, we could expose the Basix element. I'm not sure if this should be via a pointer or a reference given that not all FiniteElement objects have a Basix element.

I do think we should keep the wrapper functions for a few reasons:

  1. Wrapper makes the coupling between DOLFINx and Basix less tight, which is good
  2. It's annoying if one has to dig through the Basix documentation for finite element functionality
  3. Not all FiniteElements have a Basix element, and we may want. to handle some of these cases

@mscroggs
Copy link
Member

This looks good to me, as the only methods being taken out of the DOLFINx FiniteElement are those that are rarely used and just called Basix anyway

@nate-sime
Copy link
Contributor Author

nate-sime commented May 26, 2022

Also related: #2139, #2055, #2221.

@nate-sime
Copy link
Contributor Author

I've reverted to the previous dolfinx interface. If we're happy with this can I merge?

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.

None yet

5 participants