Fixes #148 NaNs in boolean formulas (also in If-Formulas and MinMax-Formulas) #154

Yuri05 · 2022-10-21T12:48:36Z

No description provided.

… If-Formulas and MinMax-Formulas)

msevestre · 2022-10-21T12:50:16Z

src/OSPSuite.SimModelNative/src/BooleanFormula.cpp

@@ -118,7 +120,7 @@ Formula * BooleanFormula::RecursiveSimplify()
 	if (m_FirstOperandFormula->IsConstant(CONSTANT_CURRENT_RUN)
 		&& (m_SecondOperandFormula == NULL || m_SecondOperandFormula->IsConstant(CONSTANT_CURRENT_RUN)))
 	{
-		ConstantFormula * f = new ConstantFormula( DE_Compute(NULL, 0.0, USE_SCALEFACTOR) );
+		auto f = new ConstantFormula( DE_Compute(NULL, 0.0, USE_SCALEFACTOR) );


auto is like var?

Yuri05 · 2022-10-21T12:51:56Z

There were some more formulas to fix, e.g. min/max
For test, I added the project with 3 groups of parameters:

Those which should return NaN
Those who sholuld return True (or 1)
And Those who should return False (or 0)
Currently all those highlighted parameters which must be NaN return other values.

After the fix all of them are properly set to NaN

Yuri05 · 2022-10-21T12:55:33Z

src/OSPSuite.SimModelNative/src/IfFormula.cpp

-	if (m_IfStatement->DE_Compute(y, time, USE_SCALEFACTOR) == 1)
+	auto ifStatementValue = m_IfStatement->DE_Compute(y, time, USE_SCALEFACTOR);
+	if (isnan(ifStatementValue))
+		return; //TODO is this ok? or should we throw an error instead?


here (and in other functions) it's the question for me, what should happen if NaN appears in the jacobian function:

do nothing?

throw an error?

set the corresponding Jac value to NaN?

all right,. setting to NaN is also a good option...hum
@abdullahhamadeh Any gut feel here? (This is about what do we do when we have an ? operation and the if condition evaluates to NaN ).

Actually I think it's OK AS IS (= "do nothing").
Because if we have an NaN in the conditional statement - the corresponding RHS value will be NaN anyways, which will produce the error immediately after the current RHS function evaluation is finished.
Setting additionally some Jac-Values to NaN will then have no additional effect.

Yuri05 · 2022-10-21T12:56:25Z

src/OSPSuite.SimModelNative/include/SimModel/BooleanFormula.h

-		BooleanFormula ();
-		virtual ~BooleanFormula ();
+      BooleanFormula ();
+      virtual ~BooleanFormula ();


I have to reformat the whole SimModel code once to have the proper indentation...
Will do it after 11.1

yes please. Let's createa an issue for this

Yuri05 · 2022-10-21T13:00:29Z

src/OSPSuite.SimModelNative/src/BooleanFormula.cpp

@@ -261,18 +287,17 @@ void BooleanFormula::UpdateIndicesOfReferencedVariables()

 double AndFormula::DE_Compute (const double * y, const double time, ScaleFactorUsageMode scaleFactorMode)
 {
-	assert (m_SecondOperandFormula != NULL); //first operand is always set
-	return  (m_FirstOperandFormula->DE_Compute(y, time, scaleFactorMode) && m_SecondOperandFormula->DE_Compute(y, time, scaleFactorMode));
+	return calculate_binaryOperation(y, time, scaleFactorMode, logical_and<>{});


Relevant changes are in XXX::DE_Compute() functions and in XXX::DE_Jacobian() functions (where applies)
For all boolean functions I just call calculate_binaryOperation() or calculate_unaryOperation() passing the logical operation itself as the last argument (e.g. here logical_and<>{})

msevestre

Looks good to me

msevestre · 2022-10-21T12:53:02Z

src/OSPSuite.SimModelNative/src/BooleanFormula.cpp

+
+	auto secondOperand = m_SecondOperandFormula->DE_Compute(y, time, scaleFactorMode);
+	if (isnan(secondOperand))
+		return MathHelper::GetNaN();


some of the spacing is really off here. It looks like 6 spaces?
Can't we run something like resharper and format that code once and for all lol

Yep, will try to do this after 11.1

msevestre · 2022-10-21T12:55:41Z

tests/OSPSuite.SimModel.Tests/SimulationSpecs.cs

+
+         var namePrefix = "Nan_If_Conditional_Min_Max|Organism|";
+
+         _nan_parameters = sut.ParameterProperties.Where(p=>p.Name.StartsWith($"{namePrefix}P"));


msevestre · 2022-10-21T12:57:31Z

src/OSPSuite.SimModelNative/src/IfFormula.cpp

-	if (m_IfStatement->DE_Compute(y, time, USE_SCALEFACTOR) == 1)
+	auto ifStatementValue = m_IfStatement->DE_Compute(y, time, USE_SCALEFACTOR);
+	if (isnan(ifStatementValue))
+		return; //TODO is this ok? or should we throw an error instead?


Hum...good question.
It's probably not a good case anyways.
I would not throw an error because we are NOT throwing an error on 102.

Yep, I think as well, we should not throw an error.
And if we have an NaN in the if-condition, then RHS would become NaN as well - which will be caught anyways after the solver step is finished.
So probably it's ok to keep it like this (= "do nothing") in the JAc function...

msevestre · 2022-10-21T12:59:52Z

src/OSPSuite.SimModelNative/src/BooleanFormula.cpp

 //-------------------------------------------------------------------

 double LessFormula::DE_Compute (const double * y, const double time, ScaleFactorUsageMode scaleFactorMode)
 {
-	return  (m_FirstOperandFormula->DE_Compute(y, time, scaleFactorMode) < m_SecondOperandFormula->DE_Compute(y, time, scaleFactorMode));
+	return calculate_binaryOperation(y, time, scaleFactorMode, less<>{});


this syntax is so weird less<> {}.

lol.
Actually it is less<double> {}
But VisualStudio C++ code optimization suggests to remove the template argument (double) for clarity... as for me it becomes less clear after that...
The suggestion is described here: https://releases.llvm.org/13.0.0/tools/clang/tools/extra/docs/clang-tidy/checks/modernize-use-transparent-functors.html

msevestre · 2022-10-21T13:00:52Z

src/OSPSuite.SimModelNative/src/BooleanFormula.cpp

+	return logicalOperation(firstOperand, secondOperand);
+}
+
+double BooleanFormula::calculate_unaryOperation(const double* y, const double time,


this is so cool. Lambda in C++?

yes. is actually available since C++ 14 (or even C++ 11)

msevestre · 2022-10-21T14:42:33Z

I think we can merge. looks good to me Juri

Yuri05 · 2022-10-21T14:57:03Z

ok, merged. we can update OSPSuite.Core then.
For ospsuite-R, I must create the Centos-package first

Fixes Open-Systems-Pharmacology#148 NaNs in boolean formulas (also in…

a136fab

… If-Formulas and MinMax-Formulas)

Yuri05 requested review from msevestre and PavelBal October 21, 2022 12:49

msevestre reviewed Oct 21, 2022

View reviewed changes

Yuri05 commented Oct 21, 2022

View reviewed changes

msevestre approved these changes Oct 21, 2022

View reviewed changes

Yuri05 merged commit 5b57055 into Open-Systems-Pharmacology:master Oct 21, 2022

Yuri05 deleted the 148_NaNs_in_boolean_formulas branch November 27, 2023 11:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #148 NaNs in boolean formulas (also in If-Formulas and MinMax-Formulas) #154

Fixes #148 NaNs in boolean formulas (also in If-Formulas and MinMax-Formulas) #154

Yuri05 commented Oct 21, 2022

msevestre Oct 21, 2022

Yuri05 Oct 21, 2022

Yuri05 commented Oct 21, 2022 •

edited

Loading

Yuri05 Oct 21, 2022

msevestre Oct 21, 2022

Yuri05 Oct 21, 2022

Yuri05 Oct 21, 2022

msevestre Oct 21, 2022

Yuri05 Oct 21, 2022

Yuri05 Oct 21, 2022

msevestre left a comment

msevestre Oct 21, 2022

Yuri05 Oct 21, 2022

msevestre Oct 21, 2022

msevestre Oct 21, 2022

Yuri05 Oct 21, 2022

msevestre Oct 21, 2022

Yuri05 Oct 21, 2022

msevestre Oct 21, 2022

Yuri05 Oct 21, 2022

msevestre commented Oct 21, 2022

Yuri05 commented Oct 21, 2022


		var namePrefix = "Nan_If_Conditional_Min_Max\|Organism\|";

		_nan_parameters = sut.ParameterProperties.Where(p=>p.Name.StartsWith($"{namePrefix}P"));

Fixes #148 NaNs in boolean formulas (also in If-Formulas and MinMax-Formulas) #154

Fixes #148 NaNs in boolean formulas (also in If-Formulas and MinMax-Formulas) #154

Conversation

Yuri05 commented Oct 21, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Yuri05 commented Oct 21, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msevestre left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msevestre commented Oct 21, 2022

Yuri05 commented Oct 21, 2022

Yuri05 commented Oct 21, 2022 •

edited

Loading