Skip to content

Conversation

@pbrubeck
Copy link
Contributor

@pbrubeck pbrubeck commented Jan 5, 2026

Description

Revert #4775

Validates the domains in a multidomain Form at compile time and throws a MismatchingDomainError if the domains are topologically unrelated or if the domains were not included in the integral Measure.

@pbrubeck pbrubeck force-pushed the pbrubeck/revert-domain-integral-type branch from 6b4b57f to cf4ad31 Compare January 6, 2026 21:19
@pbrubeck pbrubeck requested a review from connorjward January 7, 2026 13:45
connorjward
connorjward previously approved these changes Jan 8, 2026
@connorjward
Copy link
Contributor

Note that some tests are still failing (see the complex run).

@pbrubeck
Copy link
Contributor Author

pbrubeck commented Jan 8, 2026

Note that some tests are still failing (see the complex run).

There are some conflicts with #4763.

It seems tricky to resolve, mainly because we sometimes allow coefficients for which domain_integral_type_map[coefficient_domain] = None

@connorjward
Copy link
Contributor

Note that some tests are still failing (see the complex run).

There are some conflicts with #4763.

It seems tricky to resolve, mainly because we sometimes allow coefficients for which domain_integral_type_map[coefficient_domain] = None

I looked into this a little with @leo-collins. Setting things to None here I think is just a hack to avoid key errors later on.

Note that some tests are still failing (see the complex run).

There are some conflicts with #4763.

It seems tricky to resolve, mainly because we sometimes allow coefficients for which domain_integral_type_map[coefficient_domain] = None

Is it sufficient to just remove this line:

    domain_integral_type_map.update(dict.fromkeys(coefficient_meshes, "cell"))

This is the one that I think is properly wrong.

@pbrubeck
Copy link
Contributor Author

pbrubeck commented Jan 8, 2026

Is it sufficient to just remove this line:

    domain_integral_type_map.update(dict.fromkeys(coefficient_meshes, "cell"))

This is the one that I think is properly wrong.

That's what the PR is doing.

@connorjward
Copy link
Contributor

Is it sufficient to just remove this line:

    domain_integral_type_map.update(dict.fromkeys(coefficient_meshes, "cell"))

This is the one that I think is properly wrong.

That's what the PR is doing.

Ah OK. I didn't quite follow the logic.

@pbrubeck
Copy link
Contributor Author

I changed ValueError to NotImplementedError and complex tests are now passing. Real CI is giving a timeout, but this does not seem to be a problem introduced here.

@pbrubeck pbrubeck requested a review from connorjward January 13, 2026 13:03
connorjward
connorjward previously approved these changes Jan 13, 2026
@ksagiyam
Copy link
Contributor

#4775 basically allowed users to use illegal forms. If reverting #4775 causes conflicts with #4763, it indicates that #4763 relied on #4775 and used illegal forms in the tests; please see my comments in #4763. You don't want to manipulate types of exception just to make those tests pass. NotImplementedError is not the right type of error.

I would revert both #4763 and #4775, restore the original code, and restart from there.

You should ask @dham to have a look at these PRs.

@pbrubeck pbrubeck requested a review from connorjward January 21, 2026 10:56
@pbrubeck pbrubeck requested a review from connorjward January 21, 2026 16:57
raise NotImplementedError("Coefficient domain not found in integral. "
"Possibly, the form contains coefficients on different meshes "
"and requires measure intersection, for example: "
'Measure("dx", argument_mesh, intersect_measures=[Measure("dx", coefficient_mesh)]).')
Copy link
Member

Choose a reason for hiding this comment

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

Custom exception type here.

@dham dham merged commit b51d98d into main Jan 22, 2026
16 of 21 checks passed
@dham dham deleted the pbrubeck/revert-domain-integral-type branch January 22, 2026 16:34
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.

5 participants