Code generation for Constant/Real coefficient is flawed
For the following functionals
F = Constant(triangle)*dS
F = Constant(triangle)('+')*dS
F = Constant(triangle)('-')*dS
(is it valid UFL, @martinal?) FFC emits
w[0][0]
w[0][0]
w[0][1]
respectively. This is probably a result of first hacking Constant/Real into FFC as DG.
This is harmless when interacting with DOLFIN now, but has a funny consequence in the regression test. Test generator passes discontinuous values into interior facet integrals (even to Constant/Real
). On the other hand TSFC generates always w[0][0]
and thus shows regression in the test.
Note that this issue might have similar effects for Constant/Real as argument.
Comments (12)
-
-
reporter Can you explain why you think you need restrictions for Constant/Real?
I didn't say we need them. I said that UFL does not strip restrictions from Constants and FFC generates code for restricted Constants. This happens, e.g., for incorrectly defined
n
in this code https://bitbucket.org/fenics-project/ffc/src/fcef0ef5c334cb7237a34ade0b657658b4432c63/demo/FacetIntegrals.ufl?at=master&fileviewer=file-view-default.Maybe restrictions should be stripped/banned already at UFL level. We would need to hear @martinal 's opinion, I guess.
-
I assume
w[0][0]
andw[0][1]
contain the same value, don't they? In which case it doesn't matter whether the restriction is respected or not. -
reporter Well, they do in a real invocation by DOLFIN, but they don't in the FFC regression test.
But anyway, we should figure out what is right here and do it the same in all backends. Backends should generate (mathematically) equivalent code.
-
<deleted>
-
Sorry, forget my previous comment, I read your first line the other way around.
So I think they should be the same in the FFC regression test as well, just as in DOLFIN.
-
IMO the actual bug is in the FFC regression tests, the values should not differ. The only reason two values are passed is because UFC doesn't have actual support for global reals.
The logic to add a default restriction to a Real Coefficient is here:
I don't remember why _default_restricted is implemented as this:
def _default_restricted(self, o): "Restrict a continuous quantity to default side if no current restriction is set." r = self.current_restriction if r is None: r = self.default_restriction return o(r)
instead of just this:
def _default_restricted(self, o): "Restrict a continuous quantity to default side if no current restriction is set." return o(self.default_restriction)
Possibly just to avoid massive regression test changes, but there could be something more subtle.
-
I suspect it may just be the idea that if a restriction is in effect taking a "default restriction" of the same side might result in more subexpression reuse?
-
reporter -
assigned issue to
Agreement is to remove invocation with discontinuous constants from the test.
-
assigned issue to
-
reporter "Discontinuous constants" removed from demos by ad50649 in pull request #45. Note that tabulate_tensor test still does not have any way to ensure required continuity of passed coefficients but demos are not sensitive to it now.
-
reporter - changed milestone to 2017.1
-
reporter - changed status to resolved
Fix in master since c0b1920.
- Log in to comment
So my understanding of Constant is that it corresponds to the basis function
phi(X) = 1 in Omega
. So therefore "restricting" it is pretty meaningless (there is no "other side"). The same goes for Arguments over Real spaces, I think. As a result, in our implementation we always strip restrictions fromConstant
nodes. Can you explain why you think you need restrictions for Constant/Real?