← Back to team overview

dolfin team mailing list archive

Re: [Branch ~dolfin-core/dolfin/trunk] Rev 6896: Start cleaning up assemblers.

 

On Mon, Sep 10, 2012 at 02:26:20PM +0100, Garth N. Wells wrote:
> On Mon, Sep 10, 2012 at 2:19 PM, Anders Logg <logg@xxxxxxxxx> wrote:
> > On Mon, Sep 10, 2012 at 02:08:52PM +0100, Garth N. Wells wrote:
> >
> >> > No. Parameter names can be easily listed via
> >> >
> >> >   info(assembler.parameters, True)
> >> >
> >>
> >> That is at runtime. Not when writing code.
> >
> > Just look at the function default_parameters where they are listed?
> >
> >> > Then one gets a complete list of which parameters can be set,
> >> > including possible values for the parameters. We also get automatic
> >> > range checking.
> >> >
> >> >> Parameters are suitable for options that are passed on the command
> >> >> line, are optional and/or might be shared amongst objects.
> >> >>
> >> >> Using strings over members functions/data unnecessarily is a
> >> >> nightmare. It took a lot over work to untangle the crazy string-based
> >> >> storage that was used for parallel mesh data so that we could move the
> >> >> parallel meshes forward.
> >> >
> >> > I don't see how that is relevant.
> >>
> >> Unnecessary use of string-base naming over methods over explicit,
> >> named members and data is a poor design and makes working with code
> >> confusing.
> >
> > I completely disagree. The parameter system is good design much better
> > than cluttering classes with parameter-like variables.
> >
> > Another advantage of the parameter system is easier
> > initialization. It's a nightmare to initialize a long list of
> > parameter variables in each overloaded constructor. (Does not apply
> > here since there's only one constructor in this specific case, but I'm
> > talking general terms, see below).
> >
> >> > I very strongly think we should use
> >> > the parameter system and not start to clutter the classes with flag
> >> > member variables.
> >> >
> >>
> >> Clutter? It's four lines of code, and a lot more than that was
> >> removed. Using parameters adds more garbage.
> >
> > I agree the class in question (AssemblerBase.h/.cpp) looks very pretty
> > and neat, not much clutter there, and superbly documented.
>
> Documentation is another point in favour of member, when appropriate.
>
> > I'll give
> > you that, but it's a step away from our usual practice which is
> > avoiding member variables when we can, and sticking the parameters
> > into the parameter system.
> >
> >> I strongly oppose using the parameter system in this case because it's
> >> pointless. If someone else wants to clean up the classes and implement
> >> all the functionality that I need (and they do it this week), then I
> >> could live with the use of parameters ;).
> >
> > It's an easy fix to use the parameter system, as you say it's only 4
> > lines of code. ;-)
> >
>
> It's not just four lines because the parameter system is shaky in
> terms of casting. One cannot do:
>
>   if (parameters["foo"])
>      . . . .

I can volunteer to fix that... As usual, a function using the parameters
would start with

  // Get parameter values
  bool foo = parameters["foo"];

  ...

  if (foo)
  {

  }

--
Anders


References