Issue 58 in cyclus: Switch model copying paradigm

[cyc...@googlecode.com]

Status: Accepted
Owner: Princess...@gmail.com
Labels: Type-Maintainability Priority-Medium Component-Logic

New issue 58 by Princess...@gmail.com: Switch model copying paradigm
http://code.google.com/p/cyclus/issues/detail?id=58

Change from this:

...
old_model...
...
Model* new_model = model_creator();
new_model->copyFresh(old_model);

To this:

...
old_model...
...
Model* new_model = old_model.clone();

This eliminates the need for a separate copyFreshModel method and makes the
code more intuitive to me.

[Robert Carlsen]

I think this change would help make the code more comprehensible and cleaner (and possibly more robust). But the more I investigate, the more I am realizing how big of a change this will be.  Each copy method will return a pointer to the new model copy.  There are a couple of implementation options:

option 1:

Only the Model base class will actually allocate memory for a new model using the 'new' statement.  The copy methods of Model sub (and sub-sub) classes will invoke the copy method of their parents (in order to 'inherit' for free the contents of that copy method).  Each copy method must dynamic cast the return value of its parent's copy method into its own type (and return it).

option 2:

The initially called sub (or subsub) copy method will allocate memory (using the 'new SubSubModel' statement).  This copy method must complete the entire copy operation (including for member variables of parent classes).

option 3:

I haven't thought of it yet.

Any of these options will make the create(Model*) method in the Model class unnecessary.

[Paul Wilson]

Robert,

Maybe it's just me, but these emails are too sparse.  This behavior is all tightly bound with the notions of dynamic loading and polymorphism that we are using here.  I want to make sure we are all on the same page regarding the expected behavior before we make too many changes.

That's not to say that the current way to do it is right, but I want to make sure the changes are careful.

Paul

-- 
-- ~ ~ ~ ~ ~ ~ ~ ~ ~ ~ ~ ~ ~ ~ ~ ~ ~ ~ ~ ~ ~ ~ ~ ~ ~ ~ ~ ~ ~ ~ ~ ~ ~ --
Paul Wilson ~ UW-Madison ~ 608-263-0807 ~ cal: http://bit.ly/pphw-cal
Associate Professor, Engineering Physics. ~ http://cnerg.engr.wisc.edu
Chair, Energy Analysis & Policy Program ~ http://nelson.wisc.edu/eap

[Steve Jackson]

On Sep 23, 2011, at 8:21 , Robert Carlsen wrote:

option 1:

Only the Model base class will actually allocate memory for a new model using the 'new' statement.  The copy methods of Model sub (and sub-sub) classes will invoke the copy method of their parents (in order to 'inherit' for free the contents of that copy method).  Each copy method must dynamic cast the return value of its parent's copy method into its own type (and return it).

Option 1 won't work, if I'm reading your message right.  Suppose you have class A and class B that inherits from A.  If you define

A* A::clone(){

  A* a = new A();

  // copy this->members into a->members

  return a;

}

B* B::clone(){

  A* a = A::clone(); // call the superclass function on this object

  B* b = dynamic_cast<B*>(a); // this is an error

  ...

  return b;

}

To obtain a B instance, you must call a B constructor, otherwise the member variables of B won't exist.  Typecasting won't make them exist, it'll just halt your program, or worse.

option 2:

The initially called sub (or subsub) copy method will allocate memory (using the 'new SubSubModel' statement).  This copy method must complete the entire copy operation (including for member variables of parent classes).

This is the correct way to do it.  But subclasses don't need to (and shouldn't) copy the member variables of their parent classes.  Each class should do copying of its own members only.  The correct way to implement my above example is to use copy constructors:

A* A::clone(){

   return new A(*this);

}

B* B::clone(){

  return new B(*this);

}

The key to making this work will be in the copy constructor of B, which begins by calling the copy constructor of A:

B::B( const B& copyfrom )

: A( copyfrom )

{

    // copy B's member variables

}

~S

[Robert Carlsen]

This is the correct way to do it.  But subclasses don't need to (and shouldn't) copy the member variables of their parent classes.  Each class should do copying of its own members only.  The correct way to implement my above example is to use copy constructors:

A* A::clone(){

   return new A(*this);

}

B* B::clone(){

  return new B(*this);

}

The key to making this work will be in the copy constructor of B, which begins by calling the copy constructor of A:

B::B( const B& copyfrom )

: A( copyfrom )

{

    // copy B's member variables

}

Thank you steve - this is solution that I could "sense", but couldn't seem to quite get right.  I think the copy constructors in combination with the clone methods is the best solution.  Thoughts? 

[Katy Huff]

This seems okay to me, though this is at the level of detail I personally can only guess to be true until I see it done. 

Nonetheless, things I would be careful to watch out for when implementing this include but are not limited to: 

- accidentally copying over IDs, handles

- invalid use of templates (the first model of a certain implementation to be loaded) rather than their clones within the simulation (ask matt or paul about this. my written explanation will not be clear)

- it is necessary that we copy an object as deeply as possible, never accidentally failing to populate its implementation level (RecipeReactor) member data.

Whether these things are valid concerns will become clear during implementation. 

I recognize this was probably in your plan already, but before you code it up, I hope you'll write lots of tests for the model class reflecting issues like this, so that you can tell if you've changed any fundamental behavior.

Katy

--
http://homepages.cae.wisc.edu/~khuff/

[Robert Carlsen]

- accidentally copying over IDs, handles

Helpful advice.

 

- invalid use of templates (the first model of a certain implementation to be loaded) rather than their clones within the simulation (ask matt or paul about this. my written explanation will not be clear.

 It is clear enough.  We want a model initialized to the default state (as defined by xml) - not to the morphed state that exists in the simulation at the time of the copy.  This needs more thought.

- it is necessary that we copy an object as deeply as possible, never accidentally failing to populate its implementation level (RecipeReactor) member data.

This, however, is also an issue/concern with the current paradigm.

Or maybe, we could rethink the entire deployment concept and eliminate the need fore this sort of copying/cloning scheme all together.  We should discuss this further.

[Katy Huff]

 It is clear enough.  We want a model initialized to the default state (as defined by xml) - not to the morphed state that exists in the simulation at the time of the copy.  

quite so.

--
http://homepages.cae.wisc.edu/~khuff/