Code review comment for lp:~jobh/dolfin/symmetric-assemble

Revision history for this message
Joachim Haga (jobh) wrote :

Den 1. feb. 2012 19:50 skrev "Garth Wells" <email address hidden> følgende:
>
> I don't like terse names like
>
> Impl, lrow_is_bc, t idx, lcol, n_entries (we generally use num_foo
> in DOLFIN), etc
>
> If the names are cleaned up the code formatting is made consistent
> with the DOLFIN style, I don't have a any strong objection to merging,
> but I would prefer to use unassembled matrices. A concern is that the
> symmetric assembler code is in need of simplification, and we've made
> a few limited attempts in this direction, but the patch is making it
> more complex before it's been simplified as much as is reasonably
> possible.

Sounds good! I'll go through the parts that are new to improve naming. To
be clear, the old symmetric assemble code is not much used by the new code,
it's the standard assemble that's reused with the addition of a method to
set conditions on a cell matrix. So the old symmetric assembler can be
simplified or even removed if so desired.

It's possible to merge regular and new assembler too, with a flag for
symmetric assembly, to avoid code duplication, but it's not a completely
natural fit I think.

-j.
 Den 1. feb. 2012 19:50 skrev "Garth Wells" <email address hidden> følgende:

« Back to merge proposal