#2614 do not create logical container neighborhoods (#2615)

* #2614 do not create logical container neighborhoods

* #2614 code review

* #2614 remove formula for test failure

* #2614 test fix

* #2614 valid with warnings

* #2614 comments

Author: benjaperez1983

src/OSPSuite.Assets/UIConstants.cs

@@ -616,12 +616,12 @@ public static string UpdatedMappingsMessage(IEnumerable<(string ParameterName, s
                {
                   sb.AppendLine($"    - Output Path: {mapping.OutputPath}");
                }
+
                sb.AppendLine();
             }
 
             return sb.ToString();
          }
-
       }
 
       public static class Diff
@@ -1065,12 +1065,9 @@ public static string ParameterIdentificationFinished(string parameterIdentificat
             return $"Parameter identification '{parameterIdentificationName}' finished in {duration}";
          }
 
-
          public static string SensitivityCalculationFailed(string parameterIdentificationName, IReadOnlyList<string> errorMessages, string duration = null)
          {
-            return string.IsNullOrEmpty(duration) ? 
-               $"Parameter identification '{parameterIdentificationName}' finished but sensitivity calculation failed.\n\n {string.Join("\n\n", errorMessages)}" : 
-               $"Parameter identification '{parameterIdentificationName}' finished in {duration} but sensitivity calculation failed.\n\n {string.Join("\n\n", errorMessages)}";
+            return string.IsNullOrEmpty(duration) ? $"Parameter identification '{parameterIdentificationName}' finished but sensitivity calculation failed.\n\n {string.Join("\n\n", errorMessages)}" : $"Parameter identification '{parameterIdentificationName}' finished in {duration} but sensitivity calculation failed.\n\n {string.Join("\n\n", errorMessages)}";
          }
 
          public static string LinkedParametersIn(string name)
@@ -1316,7 +1313,7 @@ public static string SensitivityAnalysisErrorMessage(IReadOnlyList<string> error
                stringBuilder.AppendLine();
                stringBuilder.AppendLine();
             });
-            
+
             return stringBuilder.ToString();
          }
       }
@@ -1407,7 +1404,7 @@ public static class GroupRowFormat
             public static string Time = "Time";
             public static string Observation = "Observation";
             public static string DeviationLine = "Deviation Lines";
-            public static string Undefined = "Undefined"; 
+            public static string Undefined = "Undefined";
          }
 
          public static class DeviationLines
@@ -1416,7 +1413,6 @@ public static class DeviationLines
             public static string DeviationLineDescription = "Will create two deviation lines according to the given fold value which has to be greater than 1 (foldValue >1). An x-fold deviation range includes simulated values within x-fold and 1/x-fold of observed values.";
             public static string DeviationLineNameUpper(float foldValue) => $"{foldValue}-fold deviation";
             public static string DeviationLineNameLower(float foldValue) => $"{foldValue}-fold deviation Lower";
-
          }
       }
 
@@ -1425,9 +1421,9 @@ public static class DeviationLines
       private static string projectNameAndVersionAsString(string versionDisplay, int version) => $"V{versionDisplay} {numberDisplay(version)}";
 
       public static string ProjectVersionCannotBeLoaded(
-         int projectVersion, 
-         string oldestSupportedDisplayVersion, 
-         int oldestSupportedVersion, 
+         int projectVersion,
+         string oldestSupportedDisplayVersion,
+         int oldestSupportedVersion,
          string currentSupportedDisplayVersion,
          int currentSupportedVersion,
          string downloadUrl)
@@ -1451,12 +1447,12 @@ public static string ProjectVersionCannotBeLoaded(
       public static string LoadObjectFromSnapshot(string objectType) => $"Load {objectType.ToLowerInvariant()} from snapshot";
       public static string SelectSnapshotExportFile(string objectName, string objectType) => $"Export snapshot for {objectType.ToLowerInvariant()} '{objectName}'";
       public static string DoYouWantToProceed(params string[] messages) => $"WARNING:\n{messages.ToString("\n")}\n\nDo you wish to continue?";
-      
+
       public static readonly string SnapshotOfProjectWithChangedSimulationText = "Some simulations are in a changed state (red icon) and may not be re-imported correctly.";
       public static readonly string SnapshotOfProjectWithChangedSimulation = DoYouWantToProceed(SnapshotOfProjectWithChangedSimulationText);
 
       public static string Starting(string type, string name) => $"Starting {type.ToLower()} '{name}'...";
-      
+
       public static string LoadingSnapshot(string snapshotFile, string type) => $"Loading {type} from {ObjectTypes.Snapshot.ToLower()} file '{snapshotFile}'";
 
       public static string SnapshotLoaded(string typeToLoad) => $"{typeToLoad} loaded from {ObjectTypes.Snapshot.ToLower()}";
@@ -1466,7 +1462,6 @@ public static string ProjectVersionCannotBeLoaded(
       public static string StartingQualificationPlan(string qualificationPlan) => Starting(ObjectTypes.QualificationPlan, qualificationPlan);
 
       public static string StartingQualificationStep(string qualificationStep) => Starting(ObjectTypes.QualificationStep, qualificationStep);
-
    }
 
    public static class Error
@@ -1511,13 +1506,13 @@ public static string CannotFindSimulationParameterInSnapshot(string parameterPat
 
       public static string SnapshotDuplicateEntryByName(string name, string type) =>
          $"Another {type} named '{name}' already exists in the project. Snapshot file is corrupted.";
-      
+
       public static string SnapshotFileMismatch(string desiredType) => $"Snapshot file cannot be used to load a {desiredType.ToLowerInvariant()}.";
 
-      public static string CannotFindParentContainerWithPath(string parentPath, string containerName, string buildingBlockName, string moduleName) => 
+      public static string CannotFindParentContainerWithPath(string parentPath, string containerName, string buildingBlockName, string moduleName) =>
          $"Cannot find parent container '{parentPath}' defined as target of container '{containerName}' in building block '{buildingBlockName}' in module '{moduleName}'";
 
-      public static  string NoUnitColumnValues(string mappingName) => $"No values for the unit were found in the excel column mapped for '{mappingName}' \n";
+      public static string NoUnitColumnValues(string mappingName) => $"No values for the unit were found in the excel column mapped for '{mappingName}' \n";
 
       public static string ParseErrorMessage(string errors) => $"There were errors while parsing your data: {errors}";
 
@@ -1596,7 +1591,7 @@ public static string NameAlreadyExists(string name)
 
       public static string NameAlreadyExistsInContainerType(string name, string containerType)
       {
-         if(string.IsNullOrEmpty(containerType))
+         if (string.IsNullOrEmpty(containerType))
             return NameAlreadyExists(name);
 
          return $"'{name}' already exists in {containerType}.";
@@ -1738,6 +1733,7 @@ public static string MoleculeNameExistsInAnotherList(string moleculeName)
       {
          return $"Cannot add molecule '{moleculeName}' into both molecules to include and molecules to exclude lists";
       }
+
       public static string BuildingBlockTypeAlreadyAddedToModule(string objectName, string type) => $"BuildingBlock '{type}' for '{objectName}' was already added to module";
 
       public const string NotImplemented = "This feature is not implemented yet";
@@ -1888,11 +1884,11 @@ public static string TimeArrayValuesDoesNotMatchFirstIndividual(int id, int inde
 
       public static string UnitIsNotDefinedInDimension(string unit, string dimension) => $"Unit '{unit}' is not defined in dimension '{dimension}'.";
 
-      public static string CouldNotFindNeighborhoodBetween(string container1, string container2, string formulaName, string usingFormulaPath) => 
+      public static string CouldNotFindNeighborhoodBetween(string container1, string container2, string formulaName, string usingFormulaPath) =>
          $"Could not find neighborhood between '{container1}' and '{container2}' referenced by formula '{formulaName}' used by '{usingFormulaPath}'";
 
       public static string FirstNeighborNotDefinedFor(string neighborhoodName) => $"First neighbor is undefined for neighborhood '{neighborhoodName}'";
-      
+
       public static string SecondNeighborNotDefinedFor(string neighborhoodName) => $"Second neighbor is undefined for neighborhood '{neighborhoodName}'";
 
       public const string InParentTagCanOnlyBeUsedWithAndOperator = "IN PARENT tag can only be used with AND operator";
@@ -1920,6 +1916,7 @@ public static string CouldNotFindNeighborhoodBetween(string container1, string c
       public static string SimulationUsedInPlotsAreNotExported(IReadOnlyList<string> simulationNames, string project)
          => $"{ObjectTypes.Simulation.PluralizeIf(simulationNames)} {simulationNames.ToString(", ", "'")} used in plots {"is".PluralizeIf(simulationNames)} not found in the list of exported simulations for {ObjectTypes.Project} {project}";
 
+      public static string NeighborIsLogical(string neighborName, string neighborhoodName) => $"Container {neighborName} is defined as logical for neighborhood '{neighborhoodName}'";
 
       public static class SensitivityAnalysis
       {
@@ -1997,6 +1994,7 @@ private static string listErrorMessages(IReadOnlyList<string> runResultErrorMess
          {
             sb.AppendLine($"- {errorMessage}");
          }
+
          return sb.ToString();
       }
 
@@ -2223,13 +2221,12 @@ public static class Warning
       public static string LargeNumberOfOutputPoints(int numberOfPoints) =>
          $"The selected output resolution will generate {numberOfPoints} points and may severely impact the software performance.\nAre you sure you want to run with these setting? If not, consider changing output resolution in simulations settings";
 
-      public static string NeighborhoodWasNotFoundInModel(string neighborhoodName, string buildingBlockName) => $"The neighborhood '{neighborhoodName}' from building block '{buildingBlockName}' was not added to the simulation";
+      public static string NeighborhoodWasNotFoundInModel(string neighborhoodName, string buildingBlockName) => $"The neighborhood '{neighborhoodName}' from building block '{buildingBlockName}' was not added to the simulation because it is not defined or at least one of the containers is logical";
 
       public static string UnitNotFoundInDimensionForParameter(string unit, string dimension, string parameterName)
       {
          return $"Unit '{unit}' not found for parameter {parameterName} with dimension '{dimension}'";
       }
-
    }
 
    public static class RibbonCategories

src/OSPSuite.Core/Domain/Mappers/NeighborhoodBuilderToNeighborhoodMapper.cs

@@ -1,4 +1,3 @@
-using System;
 using System.Collections.Generic;
 using System.Linq;
 using OSPSuite.Core.Domain.Builder;
@@ -40,7 +39,7 @@ public NeighborhoodBuilderToNeighborhoodMapper(
          IObjectBaseFactory objectBaseFactory,
          IContainerBuilderToContainerMapper containerMapper,
          IKeywordReplacerTask keywordReplacerTask,
-         ICloneManagerForModel cloneManagerForModel, 
+         ICloneManagerForModel cloneManagerForModel,
          IParameterBuilderToParameterMapper parameterMapper, IEntityTracker entityTracker)
       {
          _objectBaseFactory = objectBaseFactory;
@@ -63,8 +62,8 @@ public Neighborhood MapFrom(NeighborhoodBuilder neighborhoodBuilder, IReadOnlyLi
          neighborhood.FirstNeighbor = resolveReference(model, neighborhoodBuilder.FirstNeighborPath, replacementContext);
          neighborhood.SecondNeighbor = resolveReference(model, neighborhoodBuilder.SecondNeighborPath, replacementContext);
 
-         //At least one neighbor cannot be found. We are ignoring this neighborhood
-         if (!neighborhood.IsDefined)
+         //At least one neighbor cannot be found or is a logical container. We are ignoring this neighborhood
+         if (!neighborhood.IsDefined || !neighborhood.HasOnlyPhysicalNeighbors)
             return null;
 
          if (neighborhoodBuilder.MoleculeProperties != null)

src/OSPSuite.Core/Domain/Neighborhood.cs

@@ -76,5 +76,7 @@ public override void UpdatePropertiesFrom(IUpdatable source, ICloneManager clone
       }
 
       public bool IsDefined => FirstNeighbor != null && SecondNeighbor != null;
+
+      public bool HasOnlyPhysicalNeighbors => IsDefined && FirstNeighbor.Mode == ContainerMode.Physical && SecondNeighbor.Mode == ContainerMode.Physical;
    }
 }

src/OSPSuite.Core/Domain/Services/SpatialStructureValidator.cs

@@ -35,9 +35,14 @@ private void validateNeighborhood(Neighborhood neighborhood, IContainer root)
          if (neighborhood.FirstNeighbor == null)
             _result.AddMessage(NotificationType.Error, neighborhood, Error.FirstNeighborNotDefinedFor(neighborhood.Name));
 
-
          if (neighborhood.SecondNeighbor == null)
             _result.AddMessage(NotificationType.Error, neighborhood, Error.SecondNeighborNotDefinedFor(neighborhood.Name));
+
+         if (neighborhood.FirstNeighbor?.Mode == ContainerMode.Logical)
+            _result.AddMessage(NotificationType.Warning, neighborhood, Error.NeighborIsLogical(neighborhood.FirstNeighbor.Name, neighborhood.Name));
+
+         if (neighborhood.SecondNeighbor?.Mode == ContainerMode.Logical)
+            _result.AddMessage(NotificationType.Warning, neighborhood, Error.NeighborIsLogical(neighborhood.SecondNeighbor.Name, neighborhood.Name));
       }
    }
 }

tests/OSPSuite.Core.IntegrationTests/ModelConstructorIntegrationTests.cs

@@ -466,7 +466,7 @@ public void should_track_the_parameter_updated_by_a_formula_accordingly()
          var parameterValue = parameterValues.First(x => x.Name == parameter.Name);
          var entitySource = _simulationBuilder.SimulationEntitySourceFor(parameter);
          entitySource.SourcePath.ShouldBeEqualTo(parameterValue.Path);
-         entitySource.SimulationEntityPath.ShouldBeEqualTo(new[] {ORGANISM, Bone, Cell, "FormulaParameterOverwritten"}.ToPathString());
+         entitySource.SimulationEntityPath.ShouldBeEqualTo(new[] { ORGANISM, Bone, Cell, "FormulaParameterOverwritten" }.ToPathString());
       }
 
       [Observation]
@@ -535,8 +535,15 @@ protected override void Context()
          var initialCondition = simulationBuilder.InitialConditions.First();
          var physicalContainer = simulationBuilder.SpatialStructureAndMergeBehaviors.SelectMany(x => x.spatialStructure.TopContainers)
             .Select(x => initialCondition.ContainerPath.TryResolve<IContainer>(x)).First(x => x != null);
-
          physicalContainer.Mode = ContainerMode.Logical;
+
+         // "Organism|ArterialBlood|Plasma" -> is being set to Logical explicitly on the previous line,
+         // as we now are not creating neighborhoods from logical containers, a further validation ends up on this exception:
+         // Could not find neighborhood between 'MyModel|Organism|ArterialBlood|Plasma' and 'MyModel|Organism|Bone|Plasma'
+         // referenced by formula 'FormulaReferencingNBH' used by 'MyModel|Organism|ArterialBlood|Plasma|RefParam'
+         // This parameter has to be removed since the validation will make this fail before testing the initial condition.
+         var parameterWithFormula = physicalContainer.Children.FirstOrDefault(x => x.Name == "RefParam") as Parameter;
+         physicalContainer.RemoveChild(parameterWithFormula);
       }
 
       [Observation]

tests/OSPSuite.Core.IntegrationTests/ModuleIntegrationTests.cs

@@ -43,7 +43,8 @@ internal class When_running_the_case_study_for_module_integration : concern_for_
       public void should_return_a_successful_validation_with_warning()
       {
          _result.ValidationResult.ValidationState.ShouldBeEqualTo(ValidationState.ValidWithWarnings, _result.ValidationResult.Messages.Select(m => m.Text).ToString("\n"));
-         _result.ValidationResult.Messages.Single().Text.ShouldBeEqualTo(Warning.NeighborhoodWasNotFoundInModel("does_not_match_existing", "Module1 - SPATIAL STRUCTURE MODULE 1"));
+         _result.ValidationResult.Messages.FirstOrDefault(x => x.Text.Equals(Warning.NeighborhoodWasNotFoundInModel("does_not_match_existing", "Module1 - SPATIAL STRUCTURE MODULE 1"))).ShouldNotBeNull();
+         _result.ValidationResult.Messages.FirstOrDefault(x => x.Text.Equals(Warning.NeighborhoodWasNotFoundInModel("not_physical", "Module1 - SPATIAL STRUCTURE MODULE 1"))).ShouldNotBeNull();
       }
 
       [Observation]
@@ -103,7 +104,7 @@ public void should_track_the_parameter_accordingly()
          _simulationBuilder.SimulationEntitySourceFor(parameter3).SourcePath.ShouldBeEqualTo(parameter3Module2.EntityPath());
       }
 
-      protected override Func<ModuleHelperForSpecs, SimulationConfiguration> SimulationConfigurationBuilder() => x => x.CreateSimulationConfiguration();
+      protected override Func<ModuleHelperForSpecs, SimulationConfiguration> SimulationConfigurationBuilder() => x => x.CreateSimulationConfigurationWithLogicalNeighborhood();
    }
 
    internal class When_running_the_case_study_for_module_integration_with_merge_behavior_extend_for_neighborhood : concern_for_ModuleIntegration

tests/OSPSuite.Core.Tests/Domain/SpatialStructureValidatorSpecs.cs

@@ -52,8 +52,14 @@ protected override void Context()
          _neighborhood1.FirstNeighbor = _liver;
          _neighborhood1.SecondNeighbor = _kidney;
 
+         _neighborhood1.FirstNeighbor.Mode = ContainerMode.Physical;
+         _neighborhood1.SecondNeighbor.Mode = ContainerMode.Physical;
+
          _neighborhood2.FirstNeighbor = _kidney;
          _neighborhood2.SecondNeighbor = _heart;
+         _neighborhood2.FirstNeighbor.Mode = ContainerMode.Physical;
+         _neighborhood2.SecondNeighbor.Mode = ContainerMode.Physical;
+
       }
 
       protected override void Because()
@@ -93,4 +99,36 @@ public void should_return_an_valid_validation_result()
          _result.ValidationState.ShouldBeEqualTo(ValidationState.Invalid);
       }
    }
+
+   internal class When_validating_a_model_for_which_some_neighborhoods_are_not_physical : concern_for_SpatialStructureValidator
+   {
+      private ValidationResult _result;
+
+      protected override void Context()
+      {
+         base.Context();
+         _neighborhood1.FirstNeighbor = _liver;
+         _neighborhood1.SecondNeighbor = _kidney;
+
+         _neighborhood1.FirstNeighbor.Mode = ContainerMode.Logical;
+         _neighborhood1.SecondNeighbor.Mode = ContainerMode.Physical;
+
+         _neighborhood2.FirstNeighbor = _kidney;
+         _neighborhood2.SecondNeighbor = _heart;
+         _neighborhood2.FirstNeighbor.Mode = ContainerMode.Physical;
+         _neighborhood2.SecondNeighbor.Mode = ContainerMode.Physical;
+
+      }
+
+      protected override void Because()
+      {
+         _result = sut.Validate(_modelConfiguration);
+      }
+
+      [Observation]
+      public void should_return_a_valid_validation_result()
+      {
+         _result.ValidationState.ShouldBeEqualTo(ValidationState.ValidWithWarnings);
+      }
+   }
 }