Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fixes #2263 #2264

Merged
merged 1 commit into from
Jun 18, 2024
Merged

Fixes #2263 #2264

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 0 additions & 5 deletions src/OSPSuite.Assets/UIConstants.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1590,11 +1590,6 @@ public static string UndefinedFormulaInHelpParameter(string parameterName, strin
return $"Cannot add help parameter '{parameterName}' with an undefined formula (null) in calculation method '{calculationMethod}' for category '{category}'";
}

public static string TwoDifferentFormulaForSameParameter(string parameter, string parameterPath)
{
return $"Formula in parameter '{parameter}' with path '{parameterPath}' was described inconsistently by more than one calculation.";
}

public static string HelpParameterAlreadyDefinedWithAnotherFormula(string calculationMethod, string parameterPath)
{
return $"There is another parameter defined at '{calculationMethod}' with another formula. Help parameter for calculation method {parameterPath} cannot be created";
Expand Down
58 changes: 11 additions & 47 deletions src/OSPSuite.Core/Domain/Services/CalculationMethodTask.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,10 @@
using System.Collections.Generic;
using System.Linq;
using OSPSuite.Assets;
using OSPSuite.Core.Domain.Builder;
using OSPSuite.Core.Domain.Descriptors;
using OSPSuite.Core.Domain.Formulas;
using OSPSuite.Core.Domain.Mappers;
using OSPSuite.Core.Extensions;
using OSPSuite.Utility.Exceptions;
using OSPSuite.Utility.Extensions;

namespace OSPSuite.Core.Domain.Services
Expand Down Expand Up @@ -48,7 +46,7 @@ public void MergeCalculationMethodInModel(ModelConfiguration modelConfiguration)
{
try
{
(_model, _simulationBuilder, _replacementContext) = modelConfiguration;
(_model, _simulationBuilder, _replacementContext) = modelConfiguration;
_simulationConfiguration = modelConfiguration.SimulationConfiguration;
_allContainers = _model.Root.GetAllContainersAndSelf<IContainer>().ToEntityDescriptorMapList();
_allBlackBoxParameters = _model.Root.GetAllChildren<IParameter>().Where(p => p.Formula.IsBlackBox()).ToList();
Expand Down Expand Up @@ -80,16 +78,14 @@ private void addHelpParametersFor(CoreCalculationMethod calculationMethod, IList
{
foreach (var container in allMoleculeContainersFor(containerDescriptor, molecule))
{
//make sure we remove the parameter if it exists already
var existingParameter = container.Parameter(helpParameter.Name);
//does not exist yet
if (existingParameter == null)
{
var parameter = _parameterMapper.MapFrom(helpParameter, _simulationBuilder);
container.Add(parameter);
replaceKeyWordsIn(parameter, molecule.Name);
}
else if (!formulasAreTheSameForParameter(existingParameter, helpParameter.Formula, molecule.Name))
throw new OSPSuiteException(Error.HelpParameterAlreadyDefinedWithAnotherFormula(calculationMethod.Name, _objectPathFactory.CreateAbsoluteObjectPath(helpParameter).ToString()));
if (existingParameter != null)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Parameter exists in the simulation? Then we remove it and add the one from the CM

container.RemoveChild(existingParameter);

var parameter = _parameterMapper.MapFrom(helpParameter, _simulationBuilder);
container.Add(parameter);
replaceKeyWordsIn(parameter, molecule.Name);
}
}
}
Expand All @@ -108,17 +104,8 @@ private void createFormulaForBlackBoxParameters(CoreCalculationMethod calculatio
if (parameterIsNotBlackBoxParameter(parameter))
continue;

//parameter formula was not set yet
if (parameter.Formula.IsBlackBox())
{
parameter.Formula = _formulaMapper.MapFrom(formula, _simulationBuilder);
replaceKeyWordsIn(parameter, molecule.Name);
}
else
{
if (!formulasAreTheSameForParameter(parameter, formula, molecule.Name))
throw new OSPSuiteException(Error.TwoDifferentFormulaForSameParameter(parameter.Name, _objectPathFactory.CreateAbsoluteObjectPath(parameter).ToPathString()));
}
parameter.Formula = _formulaMapper.MapFrom(formula, _simulationBuilder);
replaceKeyWordsIn(parameter, molecule.Name);
}
}
}
Expand All @@ -144,30 +131,7 @@ private static Neighborhood neighborhoodAncestorFor(IEntity entity)
return neighborhoodAncestorFor(entity.ParentContainer);
}

private bool formulasAreTheSameForParameter(IParameter originalParameter, IFormula calculationMethodFormula, string moleculeName)
{
var previousFormula = originalParameter.Formula;
//needs to use the parameter in order to keep the hierarchy. Hence we set the formula in the parameter
originalParameter.Formula = _formulaMapper.MapFrom(calculationMethodFormula, _simulationBuilder);

//check if the formula set are the same. it is necessary to replace keywords before doing that
replaceKeyWordsIn(originalParameter, moleculeName);

try
{
return _formulaTask.FormulasAreTheSame(originalParameter.Formula, previousFormula);
}
finally
{
//reset the origianl formula in any case
originalParameter.Formula = previousFormula;
}
}

private bool parameterIsNotBlackBoxParameter(IParameter parameter)
{
return !_allBlackBoxParameters.Contains(parameter);
}
private bool parameterIsNotBlackBoxParameter(IParameter parameter) => !_allBlackBoxParameters.Contains(parameter);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if just checking for parameter.Formula.IsBlackBox() here is not more performant than caching all black box parameters from the model into _allBlackBoxParameters and checking if a parameter is in that list every time
(Because when caching, parameter.Formula.IsBlackBox() is called for every model parameter anyway; and the function parameterIsNotBlackBoxParameter() is called only once per parameter and only for the calculation methods parameters)

Apart from that looks good for me.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

COuld be. I don't want to change the code that does not need change to fix the issue.


private IEnumerable<MoleculeBuilder> allMoleculesUsing(CoreCalculationMethod calculationMethod, IReadOnlyCollection<MoleculeBuilder> molecules)
{
Expand Down