On 22 November 2011 22:47, Anders Logg <l...@simula.no> wrote: > On Tue, Nov 22, 2011 at 10:44:07PM +0000, Garth N. Wells wrote: >> On 22 November 2011 22:13, Garth N. Wells <gn...@cam.ac.uk> wrote: >> > On 22 November 2011 22:05, Anders Logg <l...@simula.no> wrote: >> >> On Tue, Nov 22, 2011 at 10:03:19PM +0000, Garth N. Wells wrote: >> >>> On 22 November 2011 21:58, Anders Logg <l...@simula.no> wrote: >> >>> > On Tue, Nov 22, 2011 at 09:53:47PM +0000, Garth N. Wells wrote: >> >>> >> On 22 November 2011 21:50, Anders Logg <l...@simula.no> wrote: >> >>> >> > On Tue, Nov 22, 2011 at 09:33:25PM +0000, Garth N. Wells wrote: >> >>> >> >> On 22 November 2011 21:30, Anders Logg <l...@simula.no> 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"<m...@simula.no> >> >>> >> >> >> >>wrote: >> >>> >> >> >> >> >> >>> >> >> >> >>> >> >>> >> >> >> >>>On 21. nov. 2011, at 21:52, Anders Logg<l...@simula.no> >> >>> >> >> >> >>>wrote: >> >>> >> >> >> >>> >> >>> >> >> >> >>>>On Mon, Nov 21, 2011 at 08:46:13PM +0000, Garth N. Wells >> >>> >> >> >> >>>>wrote: >> >>> >> >> >> >>>>>On 21 November 2011 13:07, Anders Logg<l...@simula.no> >> >>> >> >> >> >>>>>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<l...@simula.no> >> >>> >> >> >> >>>>>>>> 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. > > ok. That makes it possible to add a check later that the input vector > makes sense. >
I think that it should still be removed (or at least made private), but it will need testing. Adding a check is not a simple solution. I'm removing the reference version of the constructor and keeping the shared_ptr version. Garth > -- > Anders > > >> Garth >> >> > Garth >> > >> > >> >> >> >> >> >>> 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. >> >>> >> > >> >>> >> > >> >>> > >> >> >> > > _______________________________________________ Mailing list: https://launchpad.net/~dolfin Post to : dolfin@lists.launchpad.net Unsubscribe : https://launchpad.net/~dolfin More help : https://help.launchpad.net/ListHelp
