2640 implement merge behavior for reactions after michaels change by benjaperez1983 · Pull Request #2725 · Open-Systems-Pharmacology/OSPSuite.Core

benjaperez1983 · 2026-01-02T10:40:49Z

Description

Implemented reaction merge aligned with the new generic merge pipeline.

Type of change

Please mark relevant options with an x in the brackets.

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
This change requires documentation changes (link at least one user or developer documentation issue):
Algorithm update - updates algorithm documentation/questions/answers etc.
Other (please describe):

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

Integration tests
Unit tests
Manual tests
No tests required

Reviewer checklist

Mark everything that needs to be checked before merging the PR.

Check if the code is well documented
Check if the behavior is what is expected
Check if the code is well tested
Check if the code is readable and well formatted
Additional checks (document below if any)
Check if documentation update issue(s) are created if the option This change requires a documentation update above is selected

Screenshots (if appropriate):

Questions (if appropriate):

I Believe on the previous pr (which is draft still) #2641
There is a comment regarding the cloning of the formula.
I have to copy it manually since the UpdatePropertiesFrom for the parameter does not carry the formula, maybe I`m messing something up.
So @msevestre keep this in mind when reviewing please.

Thanks!

src/OSPSuite.Core/Domain/Builder/SimulationBuilder.cs

…fter-michaels-change

Copilot

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

Copilot

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

src/OSPSuite.Core/Domain/Builder/SimulationBuilder.cs

tests/OSPSuite.Core.IntegrationTests/SpatialStructureSpecs.cs

src/OSPSuite.Core/Domain/Builder/SimulationBuilder.cs

Yuri05

s. comment(s)

msevestre

see comments

src/OSPSuite.Core/Domain/Builder/SimulationBuilder.cs

tests/OSPSuite.Core.IntegrationTests/SpatialStructureSpecs.cs

Copilot

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

src/OSPSuite.Core/Domain/Builder/SimulationBuilder.cs

tests/OSPSuite.Core.IntegrationTests/SpatialStructureSpecs.cs

src/OSPSuite.Core/Domain/Builder/SimulationBuilder.cs

Copilot

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

tests/OSPSuite.HelpersForTests/ModelHelperForSpecs.cs

src/OSPSuite.Core/Domain/Builder/SimulationBuilder.cs

tests/OSPSuite.Core.IntegrationTests/SpatialStructureSpecs.cs

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Copilot

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-01-15T09:38:13Z

tests/OSPSuite.Core.IntegrationTests/SpatialStructureSpecs.cs

      protected override void Context()
      {
         sut = new SpatialStructure();
+         _simulationConfiguration = IoC.Resolve<ModelHelperForSpecs>().CreateSimulationConfiguration();


Moving initialization from GlobalContext to Context may cause test isolation issues. GlobalContext runs once per test class, while Context runs before each test. This change means CreateSimulationConfiguration() will be called repeatedly, which could impact test performance and potentially cause issues if the configuration is not meant to be recreated for each test.

Suggested change

protected override void Context()

{

sut = new SpatialStructure();

_simulationConfiguration = IoC.Resolve<ModelHelperForSpecs>().CreateSimulationConfiguration();

protected override void GlobalContext()

{

base.GlobalContext();

_simulationConfiguration = IoC.Resolve<ModelHelperForSpecs>().CreateSimulationConfiguration();

}

protected override void Context()

{

sut = new SpatialStructure();

Copilot · 2026-01-15T09:38:14Z

src/OSPSuite.Core/Domain/Builder/SimulationBuilder.cs

-         //TODO add reaction merge behavior
+         var incoming = sourceReaction.Builder;
+         tryMergeContainers(targetReaction, sourceReaction);
+         targetReaction.Formula = incoming.Formula != null ? incoming.Formula.Clone() : null;


Formula cloning may not preserve all formula references and dependencies. As noted in the PR description, UpdatePropertiesFrom for parameters does not carry the formula. Ensure that formula references (e.g., object paths, formula cache entries) are properly maintained after cloning.

rwmcintosh · 2026-01-21T13:42:23Z

@msevestre can you review the changes implemented after the last round of review?

Copilot

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-01-21T14:50:24Z

src/OSPSuite.Core/Domain/Builder/SimulationBuilder.cs

+         targetReaction.Dimension = incoming.Dimension ?? targetReaction.Dimension;
+
+         if (incoming.ContainerCriteria != null)
+            targetReaction.ContainerCriteria = incoming.ContainerCriteria;


The ContainerCriteria is being directly assigned from the incoming builder, which creates a shared reference between builders. This can cause unintended side effects if either builder is modified later.

The pattern used in mergeTransports (lines 218-219) should be followed here, where mergeDescriptorCriteria is called to properly merge and clone the criteria. Alternatively, the criteria should be cloned before assignment, as done in ReactionBuilder.UpdatePropertiesFrom (line 108).

Suggested change

targetReaction.ContainerCriteria = incoming.ContainerCriteria;

targetReaction.ContainerCriteria = incoming.ContainerCriteria.Clone();

Copilot · 2026-01-21T14:50:24Z

tests/OSPSuite.Core.IntegrationTests/SpatialStructureSpecs.cs

+   internal class When_merging_reaction_with_parameters_and_formula_reference_in_integration : concern_for_SpatialStructure
+   {
+      private ModuleConfiguration _moduleConfigurationA;
+      private IDimension _amountPerTimeDimension;
+
+      private ReactionBuilder _R2;
+      private IParameter _k1;
+      private IParameter _k2;
+      private IParameter _k3;
+      private IFormula _r1Formula;
+      private IFormula _r1K3Formula;
+
+      protected override void Context()
+      {
+         base.Context();
+         var factory = IoC.Resolve<IObjectBaseFactory>();
+         _moduleConfigurationA = new ModuleConfiguration(new Module());
+         var reactionsA = new ReactionBuildingBlock();
+         _moduleConfigurationA.Module.Add(reactionsA);
+         _moduleConfigurationA.Module.MergeBehavior = MergeBehavior.Extend; 
+         _r1Formula = factory.Create<ExplicitFormula>().WithFormulaString("k1");
+         _r1K3Formula = factory.Create<ExplicitFormula>().WithFormulaString("k3");
+         var helpers = IoC.Resolve<ModelHelperForSpecs>();
+         _amountPerTimeDimension = helpers.AmountPerTimeDimension;
+
+         _R2 = new ReactionBuilder()
+            .WithName("R2")
+            .WithKinetic(_r1Formula)
+            .WithDimension(_amountPerTimeDimension);
+
+         _k1 = helpers.NewConstantParameter("k1", 22);
+         _k1.BuildMode = ParameterBuildMode.Local;
+         _R2.AddParameter(_k1);
+
+         _k2 = helpers.NewConstantParameter("k2", 1);
+         _k2.BuildMode = ParameterBuildMode.Global;
+         _R2.AddParameter(_k2);
+
+         _k3 = helpers.NewConstantParameter("k3", 1).WithFormula(_r1K3Formula);
+         _k3.BuildMode = ParameterBuildMode.Global;
+         _R2.AddParameter(_k3);
+
+         _R2.AddEduct(new ReactionPartnerBuilder("A", 1));
+         _R2.AddProduct(new ReactionPartnerBuilder("C", 1));
+
+         reactionsA.Add(_R2);
+
+         _simulationConfiguration.AddModuleConfiguration(_moduleConfigurationA);
+
+         _simulationBuilder = new SimulationBuilderForSpecs(_simulationConfiguration);
+         _simulationBuilder.PerformMerge(_simulationConfiguration);
+      }
+
+      [Observation]
+      public void should_preserve_reaction_parameters_and_build_modes()
+      {
+         var rxn = _simulationBuilder.Reactions.Single(x => x.Name == "R2");
+
+         var p1 = rxn.Parameters.Single(p => p.Name == "k1");
+         p1.Value.ShouldBeEqualTo(22);
+         p1.BuildMode.ShouldBeEqualTo(ParameterBuildMode.Local);
+
+         var p2 = rxn.Parameters.Single(p => p.Name == "k2");
+         p2.Value.ShouldBeEqualTo(1);
+         p2.BuildMode.ShouldBeEqualTo(ParameterBuildMode.Global);
+
+         var p3 = rxn.Parameters.Single(p => p.Name == "k3");
+         p3.BuildMode.ShouldBeEqualTo(ParameterBuildMode.Global);
+         p3.Formula.ShouldBeEqualTo(_r1K3Formula);
+      }
+
+      [Observation]
+      public void should_preserve_reaction_formula_and_dimension()
+      {
+         var rxn = _simulationBuilder.Reactions.Single(x => x.Name == "R2");
+         rxn.Formula.ShouldBeEqualTo(_r1Formula);
+         rxn.Dimension.ShouldBeEqualTo(_amountPerTimeDimension);
+      }
+
+      [Observation]
+      public void should_preserve_reaction_partners()
+      {
+         var rxn = _simulationBuilder.Reactions.Single(x => x.Name == "R2");
+         rxn.EductBy("A").StoichiometricCoefficient.ShouldBeEqualTo(1);
+         rxn.ProductBy("C").StoichiometricCoefficient.ShouldBeEqualTo(1);
+      }
+   }


The new integration tests do not verify ContainerCriteria merging behavior. Consider adding assertions to verify that ContainerCriteria is properly merged or cloned when reactions are extended, similar to how other properties like Formula, Dimension, and reaction partners are tested.

#2640 merge reactions and tests

555c505

Copilot AI review requested due to automatic review settings January 2, 2026 10:40

benjaperez1983 changed the base branch from develop to V13 January 2, 2026 10:41

benjaperez1983 commented Jan 2, 2026

View reviewed changes

src/OSPSuite.Core/Domain/Builder/SimulationBuilder.cs Outdated Show resolved Hide resolved

benjaperez1983 requested review from Yuri05, msevestre and rwmcintosh January 2, 2026 10:45

Merge branch 'V13' into 2640-Implement-merge-behavior-for-reacitons-a…

25d83f0

…fter-michaels-change

Copilot AI reviewed Jan 2, 2026

View reviewed changes

#2640 fix tests

0062b56

benjaperez1983 requested a review from Copilot January 2, 2026 14:14

Copilot AI reviewed Jan 2, 2026

View reviewed changes

Yuri05 reviewed Jan 2, 2026

View reviewed changes

src/OSPSuite.Core/Domain/Builder/SimulationBuilder.cs Outdated Show resolved Hide resolved

Yuri05 requested changes Jan 2, 2026

View reviewed changes

msevestre requested changes Jan 2, 2026

View reviewed changes

benjaperez1983 added 2 commits January 5, 2026 09:44

#2640 tests

3dd3fa0

#2640 changes for code review

74881ec

Copilot AI review requested due to automatic review settings January 5, 2026 11:48

Copilot AI reviewed Jan 5, 2026

View reviewed changes

#2640 reimplement logic

a469260

benjaperez1983 requested review from Yuri05 and msevestre January 5, 2026 12:18

benjaperez1983 changed the title ~~2640 implement merge behavior for reacitons after michaels change~~ 2640 implement merge behavior for reactions after michaels change Jan 7, 2026

msevestre requested changes Jan 14, 2026

View reviewed changes

src/OSPSuite.Core/Domain/Builder/SimulationBuilder.cs Outdated Show resolved Hide resolved

#2640 simulation builder change for parameres

7f02bfe

benjaperez1983 requested review from Copilot and msevestre January 15, 2026 09:26

Copilot AI reviewed Jan 15, 2026

View reviewed changes

Update src/OSPSuite.Core/Domain/Builder/SimulationBuilder.cs

ef532f1

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Copilot AI review requested due to automatic review settings January 15, 2026 09:37

Copilot AI reviewed Jan 15, 2026

View reviewed changes

#2640 rollback unintentional commit from github copilot

84cee12

msevestre requested a review from Copilot January 21, 2026 14:45

Copilot started reviewing on behalf of msevestre January 21, 2026 14:45 View session

msevestre merged commit 41fc05c into V13 Jan 21, 2026
5 checks passed

msevestre deleted the 2640-Implement-merge-behavior-for-reacitons-after-michaels-change branch January 21, 2026 14:46

Copilot AI reviewed Jan 21, 2026

View reviewed changes

	targetReaction.ContainerCriteria = incoming.ContainerCriteria;
	targetReaction.ContainerCriteria = incoming.ContainerCriteria.Clone();

Comments

Conversation

benjaperez1983 commented Jan 2, 2026

Description

Type of change

How Has This Been Tested?

Reviewer checklist

Screenshots (if appropriate):

Questions (if appropriate):

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Yuri05 left a comment

Choose a reason for hiding this comment

Uh oh!

msevestre left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Jan 15, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 15, 2026

Choose a reason for hiding this comment

Uh oh!

rwmcintosh commented Jan 21, 2026

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Jan 21, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 21, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels