RieszMap fixes/suggestions
#4568
Conversation
jrmaddison
force-pushed
the
jrmaddison/riesz_map_fixes
branch
from
ed5213a to
c04f182
Compare
| else: | ||
| solve, rhs, soln = self._solver | ||
| rhs.assign(value) | ||
| soln.zero() |
There was a problem hiding this comment.
Doesn't PETSc always use a zero initial guess unless you pass -ksp_initial_guess_nonzero?
There was a problem hiding this comment.
This might be ignored due to #4142. I'd need to check for this use case, but I think it's still safer to zero here.
There was a problem hiding this comment.
LinearSolver inherits from NonlinearVariationalSolver, which means that we are running a SNES, which gives a different behaivor from the plain KSP. Currently -ksp_initial_guess_nonzero False zeros the update to whatever is prescribed as the initial guess.
There was a problem hiding this comment.
Setting the initial guess explicitly outside PETSc is the only way with SNES.
|
@pbrubeck I think this looks fine but could you cast a quick eye over it please? |
| else: | ||
| bcs = tuple(bcs) | ||
| if len(bcs) > 0 and inner_product == "l2": | ||
| raise ValueError("Cannot supply boundary conditions with an l2 Riesz map") |
There was a problem hiding this comment.
I'm curious here. Does this lead to the wrong results in optimization?
There was a problem hiding this comment.
Does "l2" here means that we are imposing the wrong inner-product between two primal objects, or does it mean that we are pairing a primal with a dual?
There was a problem hiding this comment.
I'm curious here. Does this lead to the wrong results in optimization?
For
Does "l2" here means that we are imposing the wrong inner-product between two primal objects, or does it mean that we are pairing a primal with a dual?
Yes, it's the basis dependent map defined by dof assignment. I'm not sure if there's a case to remove this now (Firedrake code at least doesn't seem to use it much?) but maybe not for this PR?
There was a problem hiding this comment.
I'm not sure if there's a case to remove this now (Firedrake code at least doesn't seem to use it much?) but maybe not for this PR?
This close to a major release I'd say lets keep this PR to fixes, then we can think about API changes after.
Fixes based on reading the code.
Minor fixes