Skip to content

Conversation

@leo-collins
Copy link
Contributor

No description provided.

@leo-collins leo-collins requested a review from pbrubeck November 19, 2025 16:18
else:
return NotImplemented

def __matmul__(self, other):
Copy link
Contributor

@pbrubeck pbrubeck Nov 20, 2025

Choose a reason for hiding this comment

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

If this class inherits from ufl.Matrix, and ufl.Matrix is a BaseForm, shouldn't it already inherit addition and scalar multiplication?

I'm not aware of matrix multiplication being implemented in BaseForm, but it seems that it would not harm to have it there.

Copy link
Contributor

@pbrubeck pbrubeck Nov 20, 2025

Choose a reason for hiding this comment

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

Matrix multiplication is there, but it seems that it only supports multiplication times a Function (and breaks for Cofunction and Matrix).

https://github.com/FEniCS/ufl/blob/main/ufl/form.py#L247

https://github.com/FEniCS/ufl/blob/main/ufl/form.py#L219-L225

def __truediv__(self, other):
# Scalar division
if isinstance(other, Complex | Constant):
other = other.values().item() if isinstance(other, Constant) else other
Copy link
Contributor

@pbrubeck pbrubeck Nov 20, 2025

Choose a reason for hiding this comment

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

This is a bit dangerous, we are silently freezing the Constant. This returns a symbolic expression that no longer makes reference to the Constant, which is inconsistent with how scalar multiplication is implemented.

Copy link
Contributor

Choose a reason for hiding this comment

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

It might be better to leave this one for a separate PR where we enable BaseFormAssembler to numerically evaluate constant-valued symbolic expressions in the weights of a FormSum.

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