Hello,
Just to make sure I understand completely, the reason you proposed the method above over the one outlined earlier (which seemed to replace the EXPECTATION operator as it was encountered, and is outlined below for reference) is that since we are declaring auxiliary variables on the fly, and since we are creating the modfile on the fly as well, we want to be certain that we do not declare a variable for our use (in this case aux*) that the user may have referenced at a later point in their .m file.
If this is wrong and all of the symbols have in fact been declared in the SymbolTable by the time the parser begins to create expression trees, would the following be a valid procedure for implementing the Expectation operator?
Following the names EXPECTATION(iArg1)(iArg2),
in NodeID DataTree::AddExpectation(NodeID iArg1, NodeID iArg2)
* Create new auxiliary variable, auxvar(iArg1) via a separate protected function (using addSymbol, addVariable) that will find a unique variable name
* Add equation: auxvar = iArg2(-iArg1) to the model tree
If not, what is the advantage of the method proposed earlier over the one contained herein?
Also, why does the NewOperators page say we need to keep a list of new auxiliary variables? This wouldn't be necessary if the idea proposed herein is correct (but certainly would be if we were to follow the method proposed earlier).
Finally, the NewOperators page states "if the expression has not been encountered in the file, create a new auxiliary variable..." Shouldn't this new auxiliary variable be created, regardless of whether or not the expression has already been encountered?
Thanks,
Houtan
On Tue, Sep 15, 2009 at 9:21 AM, Michel Juillard
<michel.juillard@ens.fr> wrote:
Sébastien Villemot wrote:
Le lundi 14 septembre 2009 à 22:47 -0400, houtan a écrit :
I'm sorry, but I was not aware of the coding standards. Before I
received this email, I had made some edits to the code using XCode
(Mac dev tool). I have since switched to emacs and set it such that
only spaces are used. Should I svn commit everything so that when you
run the update it will take care of any tabs that may still remain in
my local version? NB: though the code compiles, the Expectation
operator is not complete.
The basic principles are that:
* each SVN commit should add a single feature or fix a single bug,
* the associated patch should be readable.
Therefore you should not modify lines which have no relation to the
purpose of the commit. And a commit which has a lot of modifications
which are only indentation changes is not very readable, since it seems
to modify a lot of lines and it is hard to detect what are the "real"
changes among the mass of indentation changes.
So in your future commits you should not fix your previous indentation
mistakes, except when you functionally change these parts.
In the near future I will run an automatic reindatation program, and
this change will be the purpose of a single commit which will *only*
consist of indentation changes.
> you are right, there is a flaw in the specification: oo_ is
not know to
> function <fname>_dynamic.m There are two solutions:
> 1) add steady_state to the list of arguments of this
function. This is
> probably more elegant. The drawback is that we increase the
number of
> arguments of the function for a rarely used feature
> 2) call Matlab global variable oo_ from inside
<fname>_dynamic.m. The
> preprocessor can add the code only for those models that
actually use
> the STEADY_STATE operator. In this case, it will be slower
than 1), but
> the code doesn't change for the majority of cases.
> What do you think, everybody?
I think I prefer solution 2): this operator is probably not
used enough
to warrant the slowdown which would probably be incurred with
solution
1).
I have modified the DataTree class such that it contains a bool
function containsSteadyStateOperator() and a protected bool
steady_state_found, which is initialized to false and set to true in
AddSteadyState(). Then, in DynamicModel.cc I write "global oo_;" only
if containsSteadyStateOperator() returns true. Is this the solution
you had in mind? Do you have any other suggestions?
I think this is ok. A cleaner way would have been to add a recursive
method "bool ExprNode::containsSteadyStateOperator()", but I don't know
if this is necessary for such a small feature.
Also, regarding eval_opcode for both the STEADY_STATE operator and the
EXPECTATION operator, should it throw an EvalException?
The current implementation of UnaryOpNode::eval_opcode() for
STEADY_STATE operator looks ok for me: since this method is only used
when evaluating the static model, it is logical to return the value
inside the operator.
For the EXPECTATION operator, let throw an EvalException, until we
decide something else.
Finally, should the derivative of the EXPECTATION operator just return
the derivative of the expression (darg2)?
We probably need to discuss the implementation of the EXPECTATION
operator.
Since the idea is to create auxiliary variables, my first thought is
that it is probably better to create a method on DataTree which would:
* walk through the expression tree and detect the expectation operators
* create the necessary auxiliary variables
* walk through the expression tree and remove the expectation operators
by replacing them by these new auxiliary variables
Then we could go through the business of the usual derivation stuff,
with no need to add special derivation rules for this operator.
Michel: does this sounds ok to you?
Best
Michel
Best,