← Back to team overview

dolfin team mailing list archive

Re: TimeDependent is too clever!

 



Anders Logg wrote:
On Mon, Jun 23, 2008 at 07:31:09PM +0100, Garth N. Wells wrote:

Dag Lindbo wrote:
Anders Logg wrote:
On Mon, Jun 23, 2008 at 02:51:05PM +0200, Dag Lindbo wrote:
Anders Logg wrote:
On Thu, Jun 19, 2008 at 11:41:52PM +0200, Dag Lindbo wrote:
Hello again!

Here comes another suggestion about the DOLFIN public interface:

Constructing time dependent functions by subclassing from Function
and
TimeDependent is very very nice. The way u.sync(t) works by storing a
pointer to t is also very clever. However, I think it is too clever

u.sync(&t)

Today I misunderstood the reference nature of sync(t), and wrote code
like
this:

while(t < T){
  "compute timestep"
  t += dt
  sync_bc(t)
}

void sync_bc(real t){
  for "all Dirichlet BCs"
     bc.sync(t)  // Pointer to stack memory gets assigned!!!!
}

:-( Of course, no one would be foolish enough to pass a pointer to
stack
memory knowingly in such a short lived context.

/Dag
TimeDependent does not seem to be used anywhere in the code so now
it's just a fancy way of storing the current time. Objects that want
to store the current time could equally well just add a member

  real t;

I suggest we remove TimeDependent for now and add it back later when
we find any use for it.

ok?
No! It is really good feature for non-kernel applications.
I think there is a clear benefit for users to have this base class to
standardize things. It is one of only a handful situations where I
really like multiple inheritance!

/Dag
I understand, but since the interface is not used in DOLFIN, it's
difficult to make any sense of how the class should behave.

The current implementation is bound to cause trouble (as you have
noticed) and I don't know how to fix it since I don't know how the
class is used.

I think that the class is worth keeping and the simple change suggested
by Dag should remove the ambiguity.

Garth
I can make the (trivial) change and add a demo under
function/TimeDependent/cpp

I just made the change, but please contribute a demo.

Garth

I'm not sure I like it but we can discuss it more later:

1. I think it should be a reference. The '&' signifies that we
may access the address of the object just as much as '*'.


I'm rather indifferent. I like the look of 'real& t' (which is why I originally did it that way), but accept that we generally pass scalars by value throughout the code which may cause some confusion. Do you suggest that we leave it as it was?

2. It's a bit strange to first send in an object to the constructor
and then call sync with a possibly new object.

It's not intended to work this way. sync is for when the time has not been declared when an object which depends on time is created.

Garth

To me, sync() should
take the time t by value (not reference).




Anyway. I'll make the new release as soon as the buildbots are happy
(currently building).



------------------------------------------------------------------------

_______________________________________________
DOLFIN-dev mailing list
DOLFIN-dev@xxxxxxxxxx
http://www.fenics.org/mailman/listinfo/dolfin-dev




Follow ups

References