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

Add C wrapper to basix for tabulation in C-code #612

Closed
wants to merge 12 commits into from

Conversation

sclaus2
Copy link

@sclaus2 sclaus2 commented Oct 20, 2022

C-wrapper for basix functions tabulate_shape and tabulate.
This C-wrapper is used to evaluate basis functions and their derivatives at a given set of points in generated C-code. This is needed for run-time quadrature evaluation used in methods such as CutFEM. This pull request contains the C-Wrapper as well as some small modifications of CMakeLists.txt in basix and is linked to #602 . @mscroggs @chrisrichardson

@@ -1,7 +1,7 @@
cmake_minimum_required(VERSION 3.16)

# Set the version
project(Basix VERSION "0.5.2.0" LANGUAGES CXX)
project(Basix VERSION "0.5.2.0" LANGUAGES CXX C)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this necessary? I'm thinking the build for Basix will still be CXX. The downstream project would use C.

https://cmake.org/cmake/help/latest/command/project.html

/// enum classes in basix
typedef struct basix_element basix_element;

basix_element* basix_element_create(const int basix_family, const int basix_cell_type, const int degree,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's super tedious, but I think to make this interface useable you will need to recreate the C++ enums as C enums.

@@ -0,0 +1,61 @@
// Copyright (c) 2022 Susanne Claus
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We use clang-format on our C++ files.

basix::element::family family = static_cast<basix::element::family>(basix_family);
basix::cell::type cell_type = static_cast<basix::cell::type>(basix_cell_type);
int k = degree;
basix::element::lagrange_variant lvariant = static_cast<basix::element::lagrange_variant>(lagrange_variant);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lvariant etc. should also be const.


extern "C" {

basix_element* basix_element_create(const int basix_family, const int basix_cell_type, const int degree,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think using int and then casting to the basix enum is OK, but I think we should explicitly add the int datatype to the class enums in basix, e.g.:

https://github.com/FEniCS/basix/blob/main/cpp/basix/element-families.h

https://en.cppreference.com/w/cpp/language/enum

"Values of integer, floating-point, and enumeration types can be converted by static_cast or explicit cast, to any enumeration type. If the underlying type is not fixed and the source value is out of range, the behavior is undefined."

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A good alternative would be to have C enum equivalents of the existing C++ enums, as mentioned above.

@@ -0,0 +1,43 @@
// Copyright (c) 2022 Susanne Claus
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add an example of use in e.g. demo/c/wrapper?

That's often helpful for getting the API looking right.

{
//Specify which element is needed
basix::element::family family = static_cast<basix::element::family>(basix_family);
basix::cell::type cell_type = static_cast<basix::cell::type>(basix_cell_type);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

reinterpret_cast might work?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Possibly not a good idea

@jhale
Copy link
Member

jhale commented Oct 21, 2022

I will fix the SonarCloud error for you on another branch.

@mscroggs mscroggs linked an issue Nov 10, 2022 that may be closed by this pull request
@garth-wells
Copy link
Member

No activity for some time, so closing.

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.

Add C-interface for some functionality
4 participants