Skip to content

Fix domain_integral_type_map - #4775

Merged
pbrubeck merged 1 commit into
mainfrom
pbrubeck/fix/domain-integral-type-map
Dec 15, 2025
Merged

pbrubeck merged 1 commit into
mainfrom
pbrubeck/fix/domain-integral-type-map

Conversation

@pbrubeck

@pbrubeck pbrubeck commented Dec 11, 2025

Copy link
Copy Markdown
Contributor

Description

integral_data.domain_integral_type_map was not encoding domains of coefficients that are not defined on the integration domain.

All domains in the form are mapped to integral_type=None, but it seems that active coefficients need integral_type="cell".

@connorjward connorjward left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is not obvious to me. Please can you amend the PR description to make it clear what this is fixing.

Comment thread tsfc/driver.py Outdated
Base automatically changed from pbrubeck/submesh-comm-self to main December 12, 2025 09:44
@pbrubeck
pbrubeck requested a review from connorjward December 12, 2025 12:22
Comment thread tsfc/driver.py
@pbrubeck
pbrubeck force-pushed the pbrubeck/fix/domain-integral-type-map branch from d4aa695 to fa7037e Compare December 12, 2025 13:55

@connorjward connorjward left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The code is pretty opaque. But it is not worse than before and it fixes a bug. I'm sufficiently happy.

@pbrubeck
pbrubeck merged commit 5ef339b into main Dec 15, 2025
7 checks passed
@pbrubeck
pbrubeck deleted the pbrubeck/fix/domain-integral-type-map branch December 15, 2025 15:25
@connorjward

Copy link
Copy Markdown
Contributor

I've realised that this fix doesn't do the right thing. You can get this test to pass without needing to modify the domain_integral_map by doing:

    mydx = Measure("dx", submesh, intersect_measures=(Measure("dx", mesh),))
    A = assemble(inner(grad(usub) * q, grad(vsub))*mydx)

@connorjward

Copy link
Copy Markdown
Contributor

I think I would be in favour of reverting this change.

@pbrubeck

Copy link
Copy Markdown
Contributor Author

I've realised that this fix doesn't do the right thing. You can get this test to pass without needing to modify the domain_integral_map by doing:

    mydx = Measure("dx", submesh, intersect_measures=(Measure("dx", mesh),))
    A = assemble(inner(grad(usub) * q, grad(vsub))*mydx)

This is interesting, and very obscure. The relationship between the measures of a submesh and its parent should be somehow implied by default, or at least we should error when we detect that case. The previous error was not that informative.

@connorjward

Copy link
Copy Markdown
Contributor

I think it is difficult to do things automatically for every case. For instance if doing an exterior facet integral over the submesh are the facets of the parent mesh interior or exterior? There are gonna be similar difficulties for facet submeshes (ie codim 1).

We could probably reasonably get things to work automatically for cell integrals specifically, because there isn't this ambiguity.

@connorjward

Copy link
Copy Markdown
Contributor

Can this be reverted?

@ksagiyam ksagiyam mentioned this pull request Jan 15, 2026
dham pushed a commit that referenced this pull request Jan 22, 2026
j-bowhay pushed a commit to j-bowhay/firedrake that referenced this pull request Feb 24, 2026
j-bowhay pushed a commit to j-bowhay/firedrake that referenced this pull request Feb 24, 2026
connorjward pushed a commit that referenced this pull request Mar 3, 2026
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.

2 participants