Pass switchEval#25
Conversation
… a more complex expression. like x = 2*sign(y)
MichaelStrik
force-pushed
the
pass-condition_value
branch
from
8fce985 to
432b8f6
Compare
mtree_ctrlifNormFormTrafo_substituteComparison
Schlevidon
left a comment
Schlevidon
left a comment
There was a problem hiding this comment.
Great work overall: the feature works and the code is much cleaner than before.
No critical bugs, but I would like to suggest some minor improvements. All of my review comments are adressed in this PR which you can just merge if you agree with my changes: #26
| function [kind, rIndex] = mtree_checkForComparisonOperator(mtreeobj, index) | ||
| % Searches the subtree of ´mtreeobj´ defined by ´index´ for nodes that are | ||
| % represent the following comparison operators: "<", ">", "<=" or ">=". |
There was a problem hiding this comment.
Comment and function name don't match the actual behavior of the function: The function only returns the type of the first comparison operator node found among the children of the index node.
The implementation of the function in general is not very clear.
| otherwise | ||
| % operator is none of the allowed types | ||
| error(config.errors.infeasible_if_condition, ... | ||
| '"If" statement does not include on of the comparisons <, >, <=, >=.'); |
There was a problem hiding this comment.
Function is missing call to config = makeConfig()
| if nargin >= 5 | ||
| to = varargin{4}; | ||
| to_type = varargin{5}; | ||
| if ~isempty(varargin) > 0 |
There was a problem hiding this comment.
The > 0 comparison is redundant. ~isempty(varargin) is sufficient.
| switch comparison_operator | ||
|
|
||
| % '<': a<b <=> not a-b>=0 | ||
| case mtreeobj.K.LT | ||
| % a<b -> a-b (replace operator by minus) | ||
| indexRightChild = mtreeobj.T( comparison_operator_index, cIndex.indexRightchild); | ||
| stringTableIndexRightChild = mtreeobj.T(indexRightChild, cIndex.stringTableIndex); | ||
|
|
||
| if (stringTableIndexRightChild ~= 0) | ||
| % Right child has a string. Include parantheses. | ||
| mtreeobj = mtree_ctrlifNormFormTrafo_substituteComparison(mtreeobj, comparison_operator_index, 0); | ||
| else | ||
| mtreeobj = mtree_ctrlifNormFormTrafo_substituteComparison(mtreeobj, comparison_operator_index, 1); | ||
| end | ||
| % That's it for now. | ||
| % preprocess_setUpCtrlif(...) takes care of the negation operator. | ||
|
|
||
|
|
||
| % '>=': a>=b <=> a-b>=0 | ||
| case mtreeobj.K.GE | ||
| % a>=b -> a-b | ||
| indexRightChild = mtreeobj.T( comparison_operator_index, cIndex.indexRightchild); | ||
| stringTableIndexRightChild = mtreeobj.T(indexRightChild, cIndex.stringTableIndex); | ||
| if ( stringTableIndexRightChild ~= 0) | ||
| % Right child has a string. Include parantheses. | ||
| mtreeobj = mtree_ctrlifNormFormTrafo_substituteComparison(mtreeobj, comparison_operator_index, 0); | ||
| else | ||
| mtreeobj = mtree_ctrlifNormFormTrafo_substituteComparison(mtreeobj, comparison_operator_index, 1); | ||
| end | ||
|
|
||
|
|
||
| % '<=': a<=b <=> b-a>=0 | ||
| case mtreeobj.K.LE | ||
| % a<=b -> b<=a | ||
| mtreeobj = mtree_switchLeftRightChildren(mtreeobj, comparison_operator_index); | ||
|
|
||
| % b<=a -> b-a | ||
| indexRightChild = mtreeobj.T( comparison_operator_index, cIndex.indexRightchild); | ||
| stringTableIndexRightChild = mtreeobj.T(indexRightChild, cIndex.stringTableIndex); | ||
| if (stringTableIndexRightChild ~= 0) | ||
| % Right child has a string. Include parantheses. | ||
| mtreeobj = mtree_ctrlifNormFormTrafo_substituteComparison(mtreeobj, comparison_operator_index, 0); | ||
| else | ||
| mtreeobj = mtree_ctrlifNormFormTrafo_substituteComparison(mtreeobj, comparison_operator_index, 1); | ||
| end | ||
|
|
||
|
|
||
| % '>': a>b <=> not b-a>=0 | ||
| case mtreeobj.K.GT | ||
| % a>=b -> b>=a | ||
| mtreeobj = mtree_switchLeftRightChildren(mtreeobj, comparison_operator_index); | ||
|
|
||
| % b>=a -> b-a | ||
| indexRightChild = mtreeobj.T( comparison_operator_index, cIndex.indexRightchild); | ||
| stringTableIndexRightChild = mtreeobj.T(indexRightChild, cIndex.stringTableIndex); | ||
|
|
||
| if (stringTableIndexRightChild ~= 0) | ||
| % Right child has a string. Include parantheses. | ||
| mtreeobj = mtree_ctrlifNormFormTrafo_substituteComparison(mtreeobj, comparison_operator_index, 0); | ||
| else | ||
| mtreeobj = mtree_ctrlifNormFormTrafo_substituteComparison(mtreeobj, comparison_operator_index, 1); | ||
| end | ||
| % Regarding the negation operator, see above. | ||
|
|
||
|
|
||
| otherwise | ||
| % operator is none of the allowed types | ||
| error(config.errors.infeasible_if_condition, ... | ||
| '"If" statement does not include on of the comparisons <, >, <=, >=.'); |
There was a problem hiding this comment.
Logic is overly complicated/redundant and can be simplified.
| % if paren==1, add parantheses | ||
| if (nargin == 2) | ||
| prths = 0; | ||
| else | ||
| prths = paren; | ||
| end |
There was a problem hiding this comment.
Awkward interface. Should either use varargin or preferably just a fixed number of arguments with a boolean flag to determine whether parantheses should be added or not.
In any case, we don't actually need this functionality, since adding additional parantheses is always safe, so I would just remove this entirely for the sake of simplicity.
| @@ -1,26 +1,34 @@ | |||
| function [mtreeobj, ctrlif_index] = mtree_replaceMinByCtrlif(mtreeobj, ctrlif_index, ignores) | |||
| % obj = replaceminByctrlif(mtreeobj) | |||
| % [mtreeobj, ctrlif_index] = mtree_replaceMinByCtrlif(mtreeobj, ctrlif_index) | |||
There was a problem hiding this comment.
Comment doesn't match function signature.
|
|
||
| % Author: | ||
| % ´mtreeobj´: Edited mtree. | ||
| % All min, min are replaced by ctrlif function calls. |
| % 'operator_type' The type of operator that the switch contains | ||
| % in its original if-statement | ||
| % (or equivalent formulation as if-statement). | ||
| % TODO: Make optional? |
There was a problem hiding this comment.
I think its fine to leave it as is. Remove this TODO.
There was a problem hiding this comment.
In general: I think the mtree_replace[Abs/If/Max/...]ByCtrlif functions have a lot of similarities, so it should be possible to simplify these a bit. However, this would most likely require some more substantial changes, so I would consider this to be out of scope for this PR. Still something to keep in mind for the future.
| function output = mtree_findBeginOfLine(mtreeobj, start_node, node_index_to_find, varargin) | ||
| % Function to either find a expr node are any other beginning of an line | ||
| function output = mtree_findBeginOfLine(mtreeobj, start_node, node_index_to_find) | ||
| % Function to either find a expr node or any other beginning of an line |
Review: pass-condition_value branch
2 participants
In preprocessed right-hand-sides, the ctrlif receives a numerical value instead of a boolean value now.
In this way, the state of (implicit) switching functions can be observed in debugging.
A RHS could look like this now:
function dx = rhs_preprocessed_if(datahandle,t,x,p)
switchEval_if_1 = 10 - x;
condition_value = ctrlif( switchEval_if_1, false, true, 1, 0, datahandle );
if condition_value
dx = 5;
else
dx = 1;
end
end
The value of the switching function associated to this ctrlif can now easily be monitored by a debugger by monitoring switchEval_if_1.
The branch is tested with speedtracker, runtime is about 10% longer compared to the snapshot 'speedtracker-latest'.
The branch also includes refactoring of functions that are part of the preprocessing.
Closes #19