Skip to content

Conversation

@sclaus2
Copy link
Contributor

@sclaus2 sclaus2 commented Dec 1, 2025

Add void* custom_data member to integral_data struct and pass it to assembly functions (matrix, vector, scalar, lifting). Custom_data defaults back to nullptr if no custom_data is set explicitly (with set_custom_data()). The required changes are minimal

  • Form.h: Add custom_data to integral_data, add accessors
  • assemble_matrix_impl.h: Pass custom_data through all functions
  • assemble_vector_impl.h: Pass custom_data through all functions
  • assemble_scalar_impl.h: Pass custom_data through all functions
  • Python bindings: Expose set_custom_data() and custom_data()
  • Add test_custom_data.py with comprehensive tests

This enables integration kernels to receive additional per-cell user data which is essential for example for runtime quadrature rules. With this extensions users are no longer required to completely rewrite all assembly routines for customised C-kernels.

For an example library that relies on custom_data and its integration into the assembler see

https://github.com/sclaus2/runintgen

@jhale
Copy link
Member

jhale commented Dec 1, 2025

Instead of void* and nullptr it would probably be better to use std::optional<void*>.

@jhale
Copy link
Member

jhale commented Dec 1, 2025

I say probably because it's a performance critical part of the code - might need a bit of thought.

@sclaus2
Copy link
Contributor Author

sclaus2 commented Dec 1, 2025

The void* approach should have minimal overhead because:

  1. The kernel function pointer signature hasn't changed, it is
    void (kernel)(T, const T*, const T*, const U*, const int*, const uint8_t*, void*)

  2. The call site simply passes the pointer (or nullptr)
    kernel(..., custom_data); // Same cost whether nullptr or valid pointer, before: null_ptr

Potential overhead is:

Storing the pointer in integral_data struct (+8 bytes per integral)
Passing the pointer to kernel (already in the signature)

I can run some performance benchmarks of this branch vs 0.10 release. If you have any existing performance benchmarks that you trust let me know.

@jhale
Copy link
Member

jhale commented Dec 1, 2025

I didn't explain myself very well. In modern C++ it's canonical to use std::optional rather than a pointer and nullptr to represent a value or absence of value. This also tends to wrap nicely into Python.

For example:

std::optional<void*> opt;
void* raw = opt.value_or(nullptr);
// pass raw up to C kernel

@schnellerhase
Copy link
Contributor

How about std::any? It is also type agnostic, but type safe, and would get rid of the undesirable void* in the source. For the kernel invocation it can be cast down to void*.

@sclaus2
Copy link
Contributor Author

sclaus2 commented Dec 1, 2025

I have changed it to std::optional<void*> for now. Open for suggestions.

@sclaus2 sclaus2 force-pushed the sclaus2/pass_custom_data_to_assembler branch from 648f439 to cf52114 Compare December 1, 2025 21:36
Replace void* with nullptr pattern with std::optional<void*> for
custom_data in the Form class and assembly functions. This is the
canonical modern C++ approach for representing optional values.

Changes:
- Form.h: integral_data::custom_data now uses std::optional<void*>
- Form.h: custom_data() returns std::optional<void*>
- Form.h: set_custom_data() accepts std::optional<void*>
- assemble_*_impl.h: Function parameters use std::optional<void*>
- assemble_*_impl.h: Kernel calls use .value_or(nullptr)
- Python bindings: Handle std::optional with None support
- Test: Update to expect None instead of 0 for unset custom_data

Benefits:
- Clearer intent: "optional value" vs "magic nullptr"
- Type safety: Distinguishes "no value" from "null pointer value"
- Pythonic: Returns None instead of 0 when not set
- Zero-cost: .value_or(nullptr) compiles to same code
@sclaus2 sclaus2 force-pushed the sclaus2/pass_custom_data_to_assembler branch from cf52114 to 8101685 Compare December 2, 2025 08:25
Copy link
Member

@jhale jhale left a comment

Choose a reason for hiding this comment

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

Looks pretty good - a few small changes to make.

@jhale
Copy link
Member

jhale commented Dec 2, 2025

How about std::any? It is also type agnostic, but type safe, and would get rid of the undesirable void* in the source. For the kernel invocation it can be cast down to void*.

I don't think nanobind supports std::any.

I'm not sure about the argument regardingvoid* being undesirable. This feature is deliberately type and memory-unsafe, and this clearly signals that the user needs to be very, very careful. I've asked @sclaus2 to add something in the docstring to that effect.

Address review comments to maintain Form immutability:
- Remove set_custom_data method from Form class
- Pass custom_data as 5th element of integrals tuple at Form creation
- Integrals tuple format: (id, kernel_ptr, entities, coeffs, custom_data)
- custom_data can be None (maps to std::nullopt) or a pointer (uintptr_t)

Pass cell index through entity_local_index for cell integrals:
- Cell integrals now receive &cell instead of nullptr for entity_local_index
- Enables per-cell data lookup in custom kernels via custom_data
- FFCx-generated kernels don't read entity_local_index for cells (return 0)

Safety documentation:
- Add warning about void pointer safety to Form.h and Python bindings
- Document that users must keep data alive while Form is in use

Update tests:
- Update test_custom_jit_kernels.py to use 5-element tuples
- Expand test_custom_data.py with per-cell material property example
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.

4 participants