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.

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

Fixes #2359 #2360

Merged
merged 4 commits into from
Nov 12, 2024
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
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
1 change: 1 addition & 0 deletions src/OSPSuite.Core/Domain/Constants.cs
Original file line number Diff line number Diff line change
Expand Up @@ -668,6 +668,7 @@ public static class ParameterExport
public const string IN_CONTAINER = "In container";
public const string NOT_IN_CONTAINER = "Not in container";
public const string IN_PARENT = "In parent";
public const string IN_CHILDREN = "In children";
public const string LLOQ = "LLOQ";

public static string NameWithUnitFor(string name, IDimension dimension) => NameWithUnitFor(name, dimension?.DefaultUnit);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,12 @@ public DescriptorCriteriaBuilder InParent()
return this;
}

public DescriptorCriteriaBuilder InChildren()
{
_criteria.Add(new InChildrenCondition());
return this;
}

public DescriptorCriteriaBuilder NotInContainer(string containerName)
{
_criteria.Add(new NotInContainerCondition(containerName));
Expand Down
22 changes: 22 additions & 0 deletions src/OSPSuite.Core/Domain/Descriptors/InChildrenCondition.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
namespace OSPSuite.Core.Domain.Descriptors
{
public class InChildrenCondition : TagCondition
{
public InChildrenCondition() : base(Constants.IN_CHILDREN, Constants.IN_CHILDREN)
{
}

public override string Condition { get; } = Constants.IN_CHILDREN.ToUpper();

//false because this condition should never be evaluated as it will need to be replaced
//by some InContainer conditions in the model
public override bool IsSatisfiedBy(EntityDescriptor item) => false;

public override ITagCondition CloneCondition() => new InChildrenCondition();

public override void Replace(string keyword, string replacement)
{
/*nothing to do*/
}
}
}
56 changes: 45 additions & 11 deletions src/OSPSuite.Core/Domain/Services/FormulaTask.cs
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
using OSPSuite.Core.Domain.Formulas;
using OSPSuite.Core.Domain.UnitSystem;
using OSPSuite.Core.Extensions;
using OSPSuite.Utility;
using OSPSuite.Utility.Collections;
using OSPSuite.Utility.Exceptions;
using OSPSuite.Utility.Extensions;
Expand Down Expand Up @@ -164,8 +165,6 @@ public void ExpandDynamicReferencesIn(IContainer rootContainer)
ExpandLumenSegmentReferencesIn(rootContainer);
}



private bool referencesNeighborhood(ObjectPath path) => path.Contains(NBH);

/// <summary>
Expand Down Expand Up @@ -234,7 +233,7 @@ internal void ExpandLumenSegmentReferencesIn(IContainer rootContainer)

private void updateLumenSegmentReferencingPath(EntityFormulaPath entityFormulaPath, IContainer rootContainer)
{
var (entity, path) = entityFormulaPath;
var (entity, path) = entityFormulaPath;
var pathAsList = path.ToList();
var firstIndex = pathAsList.IndexOf(LUMEN_SEGMENT);
var lastIndex = pathAsList.LastIndexOf(LUMEN_SEGMENT);
Expand Down Expand Up @@ -311,7 +310,7 @@ private IContainer getContainerOrThrow(IReadOnlyList<string> path, IEntity entit

public void ExpandDynamicFormulaIn(IContainer rootContainer)
{
var allFormulaUsable = rootContainer.GetAllChildren<IFormulaUsable>().ToEntityDescriptorMapList();
var allFormulaUsable = rootContainer.GetAllChildren<IFormulaUsable>();
var allEntityUsingDynamicFormula = rootContainer.GetAllChildren<IUsingFormula>(x => x.Formula.IsDynamic());

allEntityUsingDynamicFormula.Each(entityUsingFormula =>
Expand All @@ -322,31 +321,66 @@ public void ExpandDynamicFormulaIn(IContainer rootContainer)
throw new CircularReferenceInSumFormulaException(dynamicFormula.Name, entityUsingFormula.Name);

dynamicFormula.Criteria = updateDynamicFormulaCriteria(dynamicFormula, entityUsingFormula);
entityUsingFormula.Formula = dynamicFormula.ExpandUsing(allFormulaUsable, _objectPathFactory, _objectBaseFactory);

//this needs to be done after updating the criteria as dynamic tags might be added to the criteria and would
//render the descriptor map list invalid
var allFormulaUsableDescriptor = allFormulaUsable.ToEntityDescriptorMapList();
Copy link
Member Author

Choose a reason for hiding this comment

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

took me a while to figure this one out. We have some caching going on that was outside of the loop. In my implementation, I decided to add a dynamic tag to exclude specific children only (e.g. the direct children) of the container

entityUsingFormula.Formula = dynamicFormula.ExpandUsing(allFormulaUsableDescriptor, _objectPathFactory, _objectBaseFactory);
});
}

private DescriptorCriteria updateDynamicFormulaCriteria(DynamicFormula formula, IUsingFormula usingFormula)
{
//we need to replace IN PARENT criteria with actual criteria matching the parent of the usingFormula
var criteria = formula.Criteria;
var allInParentTags = criteria.Where(x => x.IsAnImplementationOf<InParentCondition>()).ToList();
var parent = usingFormula.ParentContainer;
if (!allInParentTags.Any() || parent == null)
if (parent == null)
return criteria;


var modifiedCriteria = modifyInParentFormulaCriteria(criteria, parent);
modifiedCriteria = modifyInChildrenFormulaCriteria(modifiedCriteria, parent);

return modifiedCriteria;
}

private DescriptorCriteria modifyInParentFormulaCriteria(DescriptorCriteria criteria, IContainer parent) => modifyDynamicCriteria<InParentCondition>(criteria, parent).criteria;

private DescriptorCriteria modifyInChildrenFormulaCriteria(DescriptorCriteria criteria, IContainer parent) {
var (modifyCriteria, modified) = modifyDynamicCriteria<InChildrenCondition>(criteria, parent);
if(!modified)
return criteria;

//in case of IN CHILDREN, we need to exclude the direct children from the parent from the search.
//The only way to do this is to add a condition to exclude the children explicitly. We create a random string as tag
//to avoid collision and set it in all DIRECT only children so that they will be excluded

var uniqueTag = ShortGuid.NewGuid();
Copy link
Member Author

Choose a reason for hiding this comment

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

tha'ts the gist of the logic. I create a tag, add it to all direct children and modify the criteria of the sum formula to exclude the tag explicitrely. I do not know how to do it otherwise

Copy link
Member

@Yuri05 Yuri05 Nov 12, 2024

Choose a reason for hiding this comment

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

This means that in a simulation, all direct children entities will have some dummy tags? hmm
no side effects with this after e.g. update/commit or during comparison of the simulation with BBs or so?
The tags will be also e.g. added to the simulation text report (not a big problem, but strange).

parent.GetChildren<IFormulaUsable>().Each(x => x.AddTag(uniqueTag));
modifyCriteria.Add(new NotMatchTagCondition(uniqueTag));

return modifyCriteria;
}

private (DescriptorCriteria criteria, bool modified) modifyDynamicCriteria<T>(DescriptorCriteria criteria, IContainer parent) where T : TagCondition
{
var allDynamicTags = criteria.Where(x => x.IsAnImplementationOf<T>()).ToList();

if (!allDynamicTags.Any())
return (criteria, false);

//because we need to restrict operations by adding criteria automatically, only AND makes sense
if (criteria.Operator != CriteriaOperator.And)
throw new OSPSuiteException(Error.InParentTagCanOnlyBeUsedWithAndOperator);

//we clone the criteria and remove all instances of InParentCondition. Then we add the criteria to the parent specifically
//we clone the criteria and remove all instances of the dynamic condition. Then we add the criteria to the parent specifically
var modifiedCriteria = criteria.Clone();
allInParentTags.Each(x => modifiedCriteria.RemoveByTag<InParentCondition>(x.Tag));
allDynamicTags.Each(x => modifiedCriteria.RemoveByTag<T>(x.Tag));

//add to the formula the link to parent. We use the consolidated path here so that we do not deal with the root container as criteria
var parentPath = _entityPathResolver.PathFor(parent).ToPathArray();
parentPath.Each(x => modifiedCriteria.Add(new InContainerCondition(x)));
return modifiedCriteria;

return (modifiedCriteria, true);
}

public string AddParentVolumeReferenceToFormula(IFormula formula)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,14 @@ public override void PerformMapping()
}
}

public class InChildrenConditionXmlSerializer : TagConditionXmlSerializer<InChildrenCondition>
{
public override void PerformMapping()
{
/*nothing to do*/
}
}

public class InContainerConditionXmlSerializer : TagConditionXmlSerializer<InContainerCondition>
{
}
Expand Down
86 changes: 86 additions & 0 deletions tests/OSPSuite.Core.Tests/Domain/FormulaTaskSpecs.cs
Original file line number Diff line number Diff line change
Expand Up @@ -366,6 +366,92 @@ public void should_throw_an_exception()
}
}

internal class When_expanding_all_dynamic_formula_in_a_model_using_in_children_criteria : concern_for_FormulaTask
{
private IModel _model;
private IParameter _sumVolume;
private IParameter _volume1_1;
private IParameter _volume1_2;
private IParameter _volume2_1;
private Container _container1;
private Container _container2;
private Container _subContainer1_1;
private Container _subContainer1_2;
private Container _subContainer2_1;
private Parameter _volume1;

protected override void Context()
{
base.Context();
A.CallTo(() => _objectBaseFactory.Create<ExplicitFormula>()).Returns(new ExplicitFormula());
_model = new Model();
_container1 = new Container().WithName("Container1");
_container2 = new Container().WithName("Container2");
_subContainer1_1 = new Container().WithName("SubContainer1_1").WithParentContainer(_container1);
_subContainer1_2 = new Container().WithName("SubContainer1_2").WithParentContainer(_container1);
_subContainer2_1 = new Container().WithName("SubContainer2_1").WithParentContainer(_container2);

_volume1_1 = new Parameter().WithName("volume").WithFormula(new ConstantFormula(1));
_volume1_2 = new Parameter().WithName("volume").WithFormula(new ConstantFormula(2));
_volume2_1 = new Parameter().WithName("volume").WithFormula(new ConstantFormula(3));
_volume1 = new Parameter().WithName("volume").WithFormula(new ConstantFormula(4));
_sumVolume = new Parameter().WithName("dynamic").WithFormula(new ConstantFormula(3));

//simulate a real ROOT Container
var root = new Container().WithContainerType(ContainerType.Simulation).WithName("root");
_model.Root = root;

//creating the following structure
Copy link
Member Author

Choose a reason for hiding this comment

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

@PavelBal Please check this out. I think this is what you meant but I am not sure :)

Copy link
Member

Choose a reason for hiding this comment

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

I think this is what I meant, lol :D Looks good. @msevestre

//root
// |Container1
// | |Dynamic = sum of all volume in children of Container 1
// | |volume = 4 (this parameter should not be used)
// | |SubContainer1_1
// | | |volume = 1
// | |SubContainer1_2
// | | |volume = 2
// |Container2
// | |SubContainer2_2
// | | |volume = 3 (this parameter should not be used)


root.Add(_container1);
root.Add(_container2);

_container1.Add(_sumVolume);
_container1.Add(_volume1);
_subContainer1_1.Add(_volume1_1);
_subContainer1_2.Add(_volume1_2);
//this parameter is not in the same parent and should not be used
_subContainer2_1.Add(_volume2_1);
var sumFormula = new SumFormula
{
Criteria = Create.Criteria(x => x.With("volume").InChildren())
};
_sumVolume.Formula = sumFormula;
}

protected override void Because()
{
sut.ExpandDynamicFormulaIn(_model);
}

[Observation]
public void should_replace_all_dynamic_formula_with_the_corresponding_explicit_formula()
{
_sumVolume.Formula.IsExplicit().ShouldBeTrue();
}

[Observation]
public void should_have_created_an_explicit_formula_that_is_the_sum_of_the_defined_parameters()
{
var explicitFormula = _sumVolume.Formula.DowncastTo<ExplicitFormula>();
//volume_1_1 + volume_1_2 but not volume_2_1 and not volume1
explicitFormula.FormulaString.ShouldBeEqualTo("P_1 + P_2");
explicitFormula.ObjectPaths.Count().ShouldBeEqualTo(2);
}
Comment on lines +445 to +452
Copy link
Member

Choose a reason for hiding this comment

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

so the current implementation only takes the DIRECT children into account?
I'm lost...

Copy link
Member Author

Choose a reason for hiding this comment

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

hum no

//root
         //  |Container1
         //  |  |Dynamic = sum of all volume in children of Container 1
         //  |  |volume = 4 (this parameter should not be used)
         //  |  |SubContainer1_1
         //  |  |  |volume = 1
         //  |  |SubContainer1_2
         //  |  |  |volume = 2
         //  |Container2
         //  |  |SubContainer2_2
         //  |  |  |volume = 3 (this parameter should not be used)

the parameter is defined in Container 1
so it takes SubContainer1_1 and SubContainer1_2 but not from the parent parent.
I don't have a two level check here but this is the way it would work

Copy link
Member Author

Choose a reason for hiding this comment

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

it just happens that the test does nto have SubSubContainers. I can add if you want

Copy link
Member Author

Choose a reason for hiding this comment

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

and I thought that a diagram would make everything super clear lol.

}

internal class When_replacing_the_neighborhood_keyword_in_a_well_defined_path : concern_for_FormulaTask
{
private IParameter _liverCellParameter;
Expand Down
3 changes: 1 addition & 2 deletions tests/OSPSuite.Core.Tests/Domain/InParentConditionSpecs.cs
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
using FakeItEasy;
using OSPSuite.BDDHelper;
using OSPSuite.BDDHelper;
using OSPSuite.BDDHelper.Extensions;
using OSPSuite.Core.Domain.Descriptors;

Expand Down