-
Notifications
You must be signed in to change notification settings - Fork 159
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
Dualspace update #2294
Dualspace update #2294
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fix cofunction.copy
don't introduce vectors. Introduce dats instead.
This is ready to go once UFL upstream is merged. |
@@ -139,7 +137,7 @@ def evaluate_tlm_component(self, inputs, tlm_inputs, block_variable, idx, | |||
dudmi.assign(ufl.algorithms.expand_derivatives( | |||
ufl.derivative(expr, dep.saved_output, | |||
dep.tlm_value))) | |||
dudm.vector().axpy(1.0, dudmi.vector()) | |||
dudm.dat += 1.0 * dudmi.dat |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've never seen direct operations on a PyOP2.dat
like this before. @connorjward is it the right thing to be doing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is exactly what Vector.axpy
used to do. We are just lifting the code to not rely on Vector
.
@@ -109,14 +107,14 @@ def _adj_assign_constant(self, adj_output, constant_fs): | |||
shape = r.ufl_shape | |||
if shape == () or shape[0] == 1: | |||
# Scalar Constant | |||
r.vector()[:] = adj_output.vector().sum() | |||
r.dat.data[:] = adj_output.dat.data_ro.sum() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpicking, but this I think should be r.dat.data_wo[:]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
r.dat.data[:] = adj_output.dat.data_ro.sum() | |
r.dat.data_wo[:] = adj_output.dat.data_ro.sum() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are modifying the value of r
so why should we use dat.data_ro
? .dat.data
needs to be used whenever you change the value of the .dat
, right ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apologies for the confusion, I meant to say dat.data_wo
and hastily edited the comment after making it but clearly it didn't make it to you in time! I think dat.data_wo
is correct here since you don't read the value
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, Dat.data_wo
is the same as Dat.data
(see https://github.com/OP2/PyOP2/blame/master/pyop2/types/dat.py#L223). Also, we use Dat.data
when changing the value of the Dat
all over the code so I don't think that is a problem.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well I know we've been trying to move towards using the correct access descriptors wherever we ought to. Whether or not the implementation is different @connorjward can comment on. It's just a small change but I think it's a correct one. If you're really not convinced then let @connorjward weigh in
This PR contains the full dual space implementation. It builds on top of #2034, which introduced dual function spaces, the
Cofunction
andAssembledMatrix
classes, as well as the DAG-based assembly ofBaseForm
objects.Main components of this PR:
Coargument
BaseForm
being assembled)Cofunction
in the rhs (fully annotated operation -> pyadjoint-friendly)Action
andFormSum
Cofunction
s withassign
and augmented assignmentsVector
as the default type for adjoint/tlm/hessian valuesriesz_representation
to map fromCofunction
s toFunction
s and vice-versaCofunction
s andCoargument
s now respectively have 1 and 2 arguments)The associated pyadjoint PR, which simply updates tests, can be found at dolfin-adjoint/pyadjoint#70