dolfin team mailing list archive

["Garth N. Wells" <gnw20@xxxxxxxxx>]

On 04/07/11 23:37, Anders Logg wrote:
> On Mon, Jul 04, 2011 at 11:28:56PM +0100, Garth N. Wells wrote:
>>
>>
>> On 04/07/11 17:22, Anders Logg wrote:
>>> On Mon, Jul 04, 2011 at 04:47:46PM +0100, Garth N. Wells wrote:
>>>>
>>>>
>>>> On 04/07/11 16:44, Anders Logg wrote:
>>>>> On Mon, Jul 04, 2011 at 04:39:04PM +0100, Garth N. Wells wrote:
>>>>>> I'm not sold on the NonlinearVariationalProblem interface. I would
>>>>>> prefer a constructor takes the Jacobian as an argument. It's much
>>>>>> cleaner to do things at construction and removes the need to later
>>>>>> attach the Jacobian.
>>>>>
>>>>> The point is that one should be able to define a nonlinear problem
>>>>> with or without a Jacobian. Not all nonlinear solvers need a Jacobian.
>>>>>
>>>>
>>>> That's why I wrote 'a' constructor. We can have two versions.
>>>
>>> Yes, that's an option. The drawback with that is that it would double
>>> the number of constructors (from 6 to 12) but it's a small thing to
>>> fix. I wouldn't mind moving it to the constructor.
>>>
>>
>> I think that we can rationalise the number of constructors in
>> FooVariationalProblem. For the shared pointer versions, it would be
>> enough to have just
>>
>>     LinearVariationalProblem(boost::shared_ptr<const Form> a,
>>          boost::shared_ptr<const Form> L,
>>          boost::shared_ptr<Function> u,
>>          std::vector<boost::shared_ptr<const BoundaryCondition> > bcs);
>>
>> and
>>
>>     NonlinearVariationalProblem(boost::shared_ptr<const Form> F,
>>          boost::shared_ptr<const Form> J,
>>          boost::shared_ptr<Function> u,
>>          std::vector<boost::shared_ptr<const BoundaryCondition> > bcs);
> 
> I think it's important that we keep the same argument order as for the
> reference versions, and that the Jacobian comes last to emphasize
> that it is an optional/auxiliary argument. It also removes some of the
> confusion we had be for with (a, L) vs (F, J). How about this:
> 
>   LinearVariationalProblem(boost::shared_ptr<const Form> a,
>                            boost::shared_ptr<const Form> L,
>                            boost::shared_ptr<Function> u,
>                            std::vector<boost::shared_ptr<const
>                            BoundaryCondition> > bcs);
> 
>   NonlinearVariationalProblem(boost::shared_ptr<const Form> F,
>                               int rhs,
>                               boost::shared_ptr<Function> u,
>                               std::vector<boost::shared_ptr<const
>                               BoundaryCondition> > bcs,
>                               boost::shared_ptr<const Form> J);
> 
> Same as your suggestion (just one shared ptr constructor in each
> class) but placing the Jacobian last and keeping the right-hand side
> in there.
> 

I'm don't mind the order being the same (I didn't pay any attention to
it). My main point is that the shared_ptr interface is lower level so we
don't need to provide multiple convenience versions.

> Keeping the right-hand side is important since it makes it possible to
> check for errors (like a nonzero right-hand side) in one single
> place (inside NonlinearVariationaProblem.cpp). We would otherwise need
> to check it inside Equation.cpp and possibly in the Python layer.
> 

I don't like it. It makes the nonlinear interface clumsy. Reading the
signature it's not clear to me what it's for or what I should pass in.

Garth

> --
> Anders
> 
> 
>> Garth
>>
>>>>> I agree that it's in general cleaner to require as much data as
>>>>> possible at the time of construction, but think that the handling of
>>>>> the Jacobian data is quite clean: it's a shared pointer that may be
>>>>> null and the nonlinear solver can call has_jacobian to check whether
>>>>> it has been specified.
>>>>>
>>>>
>>>> Which we can still do with two constructors.