dolfin team mailing list archive

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

On 05/07/11 07:24, Anders Logg wrote:
> On Mon, Jul 04, 2011 at 11:46:00PM +0100, Garth N. Wells wrote:
>>
>>
>> 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.
> 
> Agree.
> 
>>> 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.
> 
> The point is that it makes the interface for all variational problems
> (linear or nonlinear) the same:
> 
>   lhs, rhs, solution, [bcs], [jacobian]
> 
> I think this is helpful to minimize errors.
>

I don't agree. It's confusing. It's not clear to me why I would want to
pass a pointless integer to a NonlinearVariationalProblem. I wouldn't
expect a pointless argument, so I would start to wonder what it should
be and what it's for.

Garth


> The docstrings in the FooVariationalProblem classes can be expanded to
> explain the arguments in more detail.
> 
> --
> Anders