dolfin team mailing list archive

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

On 22 November 2011 22:13, Garth N. Wells <gnw20@xxxxxxxxx> wrote:
> On 22 November 2011 22:05, Anders Logg <logg@xxxxxxxxx> wrote:
>> On Tue, Nov 22, 2011 at 10:03:19PM +0000, Garth N. Wells wrote:
>>> On 22 November 2011 21:58, Anders Logg <logg@xxxxxxxxx> wrote:
>>> > On Tue, Nov 22, 2011 at 09:53:47PM +0000, Garth N. Wells wrote:
>>> >> On 22 November 2011 21:50, Anders Logg <logg@xxxxxxxxx> wrote:
>>> >> > On Tue, Nov 22, 2011 at 09:33:25PM +0000, Garth N. Wells wrote:
>>> >> >> On 22 November 2011 21:30, Anders Logg <logg@xxxxxxxxx> wrote:
>>> >> >> > On Tue, Nov 22, 2011 at 10:16:54PM +0100, Marie E. Rognes wrote:
>>> >> >> >> On 11/22/2011 09:55 PM, Anders Logg wrote:
>>> >> >> >> >On Tue, Nov 22, 2011 at 08:45:30PM +0000, Garth N. Wells wrote:
>>> >> >> >> >>
>>> >> >> >> >>On 21 Nov 2011, at 21:53, "Marie E. Rognes"<meg@xxxxxxxxx>  wrote:
>>> >> >> >> >>
>>> >> >> >> >>>
>>> >> >> >> >>>On 21. nov. 2011, at 21:52, Anders Logg<logg@xxxxxxxxx>  wrote:
>>> >> >> >> >>>
>>> >> >> >> >>>>On Mon, Nov 21, 2011 at 08:46:13PM +0000, Garth N. Wells wrote:
>>> >> >> >> >>>>>On 21 November 2011 13:07, Anders Logg<logg@xxxxxxxxx>  wrote:
>>> >> >> >> >>>>>>On Sun, Nov 20, 2011 at 11:55:43PM +0100, Anders Logg wrote:
>>> >> >> >> >>>>>>>On Sun, Nov 20, 2011 at 11:49:42PM +0100, Marie E. Rognes wrote:
>>> >> >> >> >>>>>>>>
>>> >> >> >> >>>>>>>>On 20. nov. 2011, at 23:31, Anders Logg<logg@xxxxxxxxx>  wrote:
>>> >> >> >> >>>>>>>>
>>> >> >> >> >>>>>>>>>Is anyone using the Function constructor that takes a vector as input
>>> >> >> >> >>>>>>>>>argument?
>>> >> >> >> >>>>>>>>>
>>> >> >> >> >>>>>>>>>Function u(V, x);
>>> >> >> >> >>>>>>>>>
>>> >> >> >> >>>>>>>>Yes.
>>> >> >> >> >>>>>>>Does it work? In parallel?
>>> >> >> >> >>>>>>>
>>> >> >> >> >>>>>>>Does it not work to instead use
>>> >> >> >> >>>>>>>
>>> >> >> >> >>>>>>>  x = u.vector()
>>> >> >> >> >>>>>>>
>>> >> >> >> >>>>>>>?
>>> >> >> >> >>>>>>>
>>> >> >> >> >>>>>>>If you need it, we should keep it but add an error message that it
>>> >> >> >> >>>>>>>doesn't work in parallel, unless it does...
>>> >> >> >> >>>>>>Any more input on this? There are several options:
>>> >> >> >> >>>>>>
>>> >> >> >> >>>>>>1. Remove this constructor
>>> >> >> >> >>>>>>
>>> >> >> >> >>>>>>2. Throw an error when running in parallel
>>> >> >> >> >>>>>>
>>> >> >> >> >>>>>>3. Check that the input vector makes sense
>>> >> >> >> >>>>>>
>>> >> >> >> >>>>>>The last one is problematic since I don't see an easy way to perform
>>> >> >> >> >>>>>>the check, other than calling get_local and having it fail.
>>> >> >> >> >>>>>>
>>> >> >> >> >>>>>I haven't heard any reason why it can't be removed. We may need to fix
>>> >> >> >> >>>>>assignment (re earlier discussion on assign) to just copy values and
>>> >> >> >> >>>>>not the whole object so that a user can get the vector and then assign
>>> >> >> >> >>>>>values to it without messing up the ghosting.
>>> >> >> >> >>>>Sounds good, but I want to wait for Marie to comment before I remove
>>> >> >> >> >>>>it. She is using it.
>>> >> >> >> >>>>
>>> >> >> >> >>>>Marie? Does it work for you to use x = u.vector()?
>>> >> >> >> >>>>
>>> >> >> >> >>>Probably. However removing the constructor would be changing parts of the basic interface, which I think is a bad idea.
>>> >> >> >> >>>
>>> >> >> >> >>>Add a warning if you want to deprecate it later.
>>> >> >> >> >>>
>>> >> >> >> >>Isn't the time to make an interface change now rather than later?
>>> >> >> >>
>>> >> >> >> I would say that the time to make an interface change before
>>> >> >> >> 1.0 has passed: I see more value in sticking to
>>> >> >> >> to what we have claimed, than in fixing this single instance.
>>> >> >> >>
>>> >> >> >> >True, but last time we discussed this was 1 hour or so before the
>>> >> >> >> >release of 1.0-rc1. Now we have a whole week to 1.0-rc2... :-)
>>> >> >> >> >
>>> >> >> >> >Marie, can you check again if that constructor is necessary?
>>> >> >> >>
>>> >> >> >> I'm typically using it for the same as the dolfin la/eigenvalue demo
>>> >> >> >> is using it for.
>>> >> >> >> Do you have a replacement syntax available?
>>> >> >
>>> >> > Marie, I think this should work:
>>> >> >
>>> >> >  u = Function(V)
>>> >> >  u.vector()[:] = x
>>> >> >
>>> >> > where x is the solution you get from the eigenvalue problem.
>>> >> >
>>> >> > Can you see if that works?
>>> >> >
>>> >>
>>> >> It won't,  because of a bad flaw in the vector assignment. It will
>>> >> make u.vector()[:] a copy of x (which has the wrong parallel layout) ,
>>> >> when what we want is to assign just the values.
>>> >
>>> > I thought you argued before that was the correct behavior? (When we
>>> > discussed the subfunction assignment last week.)
>>> >
>>>
>>> I argued that it is being done consistently, but not that it's right.
>>> We need to distinguish between copying and object and assigning just
>>> values of a vector (but not touching the layout, ghost values, etc) to
>>> another vector of the same length.
>>
>> Agree.
>>
>>> > Is it much work to fix that before 1.0-rc2?
>>> >
>>>
>>> Proper assignment is too much work. I'm adding a work-around fix now
>>> so the eigenvalue demo should be correct in parallel.
>>
>> What are the implications for whether we should remove or keep that
>> constructor. We should only remove it if we can provide a working
>> alternative (assignment operator working).
>>
>
> We can remove the constructor and provide an alternative that is no
> more broken that what we have now, but which has scope for an easy fix
> later.
>

The constructors are used correctly in a couple of places inside the
library, so removing them will require some changes and testing which
it's too late for now. I've made some tiny changes to two demos to
avoid using these constructors incorrectly, and I'll add a warning to
the docstring that the constructors are intended for internal library
usage only.

Garth

> Garth
>
>
>> --
>> Anders
>>
>>
>>> Garth
>>>
>>> >
>>> >
>>> >> Garth
>>> >>
>>> >> >
>>> >> >
>>> >> >
>>> >> >> >> That said, I'm not going to lose any sleep over this.
>>> >> >> >
>>> >> >> > Is everyone ok with throwing an error that it doesn't work in
>>> >> >> > parallel?
>>> >> >> >
>>> >> >>
>>> >> >> I don't think that is ideal.
>>> >> >>
>>> >> >> I building now with the constructors commented out to see how many
>>> >> >> changes would be required.
>>> >> >
>>> >> >
>>> >
>>
>