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 _______________________________________________ Mailing list: https://launchpad.net/~dolfin Post to : dolfin@lists.launchpad.net Unsubscribe : https://launchpad.net/~dolfin More help : https://help.launchpad.net/ListHelp
