Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Missing constant handlers #3224

Closed
Ig-dolci opened this issue Nov 10, 2023 · 10 comments · Fixed by #3261
Closed

Missing constant handlers #3224

Ig-dolci opened this issue Nov 10, 2023 · 10 comments · Fixed by #3261
Assignees
Labels

Comments

@Ig-dolci
Copy link
Contributor

Describe the bug
Missing handled for Constant

Steps to Reproduce
Steps to reproduce the behavior:

from firedrake_adjoint import *
continue_annotation()

n = 100
mesh = UnitIntervalMesh(n)
V = FunctionSpace(mesh, "CG", 2)
end = 0.01
timestep = Constant(1.0/n)
steps = int(end/float(timestep)) + 1

def Dt(u, u_, timestep):
    return (u - u_)/timestep

x, = SpatialCoordinate(mesh)
ic = project(sin(2.*pi*x), V)
u_ = Function(V)
u = Function(V)
v = TestFunction(V)
u_.assign(ic)
nu = Constant(0.0001)
F = (Dt(u, u_, timestep)*v
        + u*u.dx(0)*v + nu*u.dx(0)*v.dx(0))*dx
bc = DirichletBC(V, 0.0, "on_boundary")

solve(F == 0, u, bc)

tape = get_working_tape()
tape.visualise("tape.pdf")

Expected behavior
Describe what you expected to happen, in the case of a regression,
include details of a setup where this behaviour was last seen.

Error message

  File "/Users/ddolci/work/firedrake_main/src/ufl/ufl/corealg/map_dag.py", line 98, in map_expr_dags
    r = handlers[v._ufl_typecode_](v)
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/ddolci/work/firedrake_main/src/ufl/ufl/formatting/ufl2unicode.py", line 813, in expr
    raise ValueError(f"Missing handler for type {type(o)}")
ValueError: Missing handler for type <class 'firedrake.constant.Constant'>

Environment:

  • OS: MacOS
  • Python version: 3.11.6

Additional Info
Write a Constant handle within Firedrake that can fix this error.

@Ig-dolci Ig-dolci added the bug label Nov 10, 2023
@Ig-dolci Ig-dolci self-assigned this Nov 10, 2023
@nbouziani
Copy link
Contributor

nbouziani commented Nov 18, 2023

ufl2unicode doesn't currently work with BaseForm objects and Constant as well, as discussed in the following UFL issue firedrakeproject/ufl#43, which breaks tape visualisation. I will close that issue as this is a UFL problem and there is already an opened issue.

@connorjward
Copy link
Contributor

The missingConstant handler is actually a Firedrake problem as it is a class that we implement outside of UFL. I think this issue should remain open.

@connorjward connorjward reopened this Nov 18, 2023
@nbouziani
Copy link
Contributor

I don't think the fact that Firedrake has its own Constant object implies that this is a Firedrake problem. ufl2unicode is a UFL -specific operation so it should be handled at the UFL level, by directly handling the UFL parent class of the Firedrake constant object. In fact, that's what we already do for Function, right ?

I have also just realised that this is what we used to do for Constant as well and it used to work, but the recent constant changes (see #2927) changed the dependency (i.e. it now subclasses ConstantValue and not Coefficient).

@nbouziani
Copy link
Contributor

So I think this PR should be closed and another one should be opened in UFL to address firedrakeproject/ufl#43. I will try to do it soon.

@JDBetteridge
Copy link
Member

There is a temporary workaround in #3261

@JDBetteridge JDBetteridge linked a pull request Nov 23, 2023 that will close this issue
@connorjward
Copy link
Contributor

Unfortunately there is not really a suitable UFL parent class that encapsulates what a Firedrake Constant is. For instance our Constant has a count attribute (that we would want in ufl2unicode) but other ConstantValue objects do not.

I believe that we really need a UFL Parameter class but that doesn't exist.

@nbouziani
Copy link
Contributor

I see. Is there any obstacle in having a Parameter class that subclasses both ConstantValue and Counted, which could then be subclassed by firedrake.Constant ? That really seems to be a UFL problem, so I would advocate having a fix at the UFL level. However, I don't have a strong opinion on that and if it is a problem that needs to be fixed imminently, I guess we can go with the workaround.

@connorjward
Copy link
Contributor

I would love a proper fix for this in UFL. And perhaps adding this Parameter class is the way to go. I've been hesitant to tackle this because getting the abstraction right is hard. We don't currently have a good approach for differentiating Constants for example and I don't know how well that operation is defined.

Basically everyone knows that Constants are a bit dodgy and I don't want to give them any unearned legitimacy by putting them into UFL without serious thought.

@dham
Copy link
Member

dham commented Nov 23, 2023

See also very similar discussion on FEniCS slack: https://fenicsproject.slack.com/archives/C9EQQU29M/p1699625183307119

@connorjward
Copy link
Contributor

See also very similar discussion on FEniCS slack: https://fenicsproject.slack.com/archives/C9EQQU29M/p1699625183307119

Interesting! Good to know this is a more general problem.

@dham dham closed this as completed in #3261 Mar 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants