Code generation for Constant/Real coefficient is flawed

[Jan Blechta]

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.

[Lawrence Mitchell]

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 from Constant nodes. Can you explain why you think you need restrictions for Constant/Real?

[Jan Blechta]

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.

[Miklós Homolya]

I assume w[0][0] and w[0][1] contain the same value, don't they? In which case it doesn't matter whether the restriction is respected or not.

[Jan Blechta]

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.

[Miklós Homolya]

<deleted>

[Miklós Homolya]

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.

[Martin Sandve Alnæs]

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:

https://bitbucket.org/fenics-project/ufl/src/88675dcdf9f19b1e012487d39b0dac56cd8e46f7/ufl/algorithms/apply_restrictions.py?at=master&fileviewer=file-view-default#apply_restrictions.py-122

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.

[Lawrence Mitchell]

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?

[Jan Blechta]

Agreement is to remove invocation with discontinuous constants from the test.

[Jan Blechta]

"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.

[Jan Blechta]

Fix in master since c0b1920.