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

Riesz representative changed from l2 to L2 #3576

Closed
wants to merge 1 commit into from

Conversation

APaganini
Copy link
Contributor

Description

Ensure that default Riesz representative is L2 and not l2 by changing line 228 of firedrake/adjoint_utils/function.py from

riesz_representation = options.get("riesz_representation", "l2")
to

riesz_representation = options.get("riesz_representation", "L2")
This fixes #3575

@APaganini APaganini changed the title changed l2 into L2 Riesz representative changed from l2 to L2 May 17, 2024
@APaganini APaganini requested a review from dham May 17, 2024 16:18
@Ig-dolci
Copy link
Contributor

Thank you for catching this bug! Perhaps a test can be added to this PR? I just wrote this:

    # Make sure the derivative provides the right computation 
    # for the Riesz representation with the H1, L2, l2 and None options.
    mesh = UnitIntervalMesh(1)
    space = FunctionSpace(mesh, "CG", 1)
    f = Function(space)
    x = SpatialCoordinate(mesh)
    f.interpolate(x[0])
    J = assemble((f ** 2) * dx)
    with stop_annotating():
        v = TestFunction(space)
        u = TrialFunction(space)
        dJdu_cofunction = assemble(2 * inner(f, v) * dx)

        # Riesz representation with l2
        dJdu_function_l2 = Function(space, val=dJdu_cofunction.dat)

        # Riesz representation with H1
        a = firedrake.inner(u, v)*firedrake.dx \
            + firedrake.inner(firedrake.grad(u), firedrake.grad(v))*firedrake.dx
        dJdu_function_H1 = Function(space)
        solve(a == dJdu_cofunction, dJdu_function_H1)

        # Riesz representation with L2
        a = firedrake.inner(u, v)*firedrake.dx
        dJdu_function_L2 = Function(space)
        solve(a == dJdu_cofunction, dJdu_function_L2)

    rf = ReducedFunctional(J, Control(f))
    assert np.allclose(
        rf.derivative(options={"riesz_representation": None}).dat.data_ro, dJdu_cofunction.dat.data_ro
        )
    assert np.allclose(
        rf.derivative(options={"riesz_representation": "H1"}).dat.data_ro, dJdu_function_H1.dat.data_ro
        )
    assert np.allclose(
        rf.derivative(options={"riesz_representation": "l2"}).dat.data_ro, dJdu_function_l2.dat.data_ro
        )
    # L2 is the default option
    assert np.allclose(rf.derivative().dat.data_ro, dJdu_function_L2.dat.data_ro)

@Ig-dolci
Copy link
Contributor

This PR will be closed as PR 3579 addresses the same changes.

@Ig-dolci Ig-dolci closed this May 23, 2024
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.

BUG: Riesz representation default options is not L2 but l2
2 participants