#2640 implement merge behavior on Reactions by benjaperez1983 · Pull Request #2641 · Open-Systems-Pharmacology/OSPSuite.Core

benjaperez1983 · 2025-09-16T15:16:02Z

Description

Merge behavior for reactions

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):

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

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

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

msevestre

ok some feedback. This will change completely once the merge stuff that I am doing is implemented. For nwo my suggestgion would be to have one method that takes two reaction, a merge and returns the merge results.. This should be tested and this shoudl be enough.

overall the code is veyr hard to read. Did you generate this code with AI somehow?

tests/OSPSuite.Core.IntegrationTests/ModelConstructorIntegrationTests.cs

benjaperez1983 · 2025-09-17T07:54:10Z

ok some feedback. This will change completely once the merge stuff that I am doing is implemented. For nwo my suggestgion would be to have one method that takes two reaction, a merge and returns the merge results.. This should be tested and this shoudl be enough.

overall the code is veyr hard to read. Did you generate this code with AI somehow?

I have resolved the comments how, I have some remarks anyways.

On the Cloning, you are right there is a whole logic for it (which I`m reusing right now) but nevertheless there are some things that are not being fully cloned (such as formula and parameters), so I added that to it.

Regarding your other question, funny thing I had a very large and difficult to read nested methods which I asked the AI to simplify, turned out to be even harder to read, so I rolled that back and try refactoring in a way it is better now.
So please feel free to go through it again and let me know.

Thanks!

msevestre · 2025-09-29T03:17:41Z

@benjaperez1983 Let's meet when you have time this week. Most of this code is obsolete I believe. I am going to comment on the PR what I believe will be kept

msevestre

I suggest to close this PR and to make one towards #2603 implementing the mergeReactions method

     private void mergeReactions(ReactionBuilder target, BuilderSource<ReactionBuilder> source)
     {
        //TODO add reaction merge behavior
     }

integrations tests should be written by modifying the ModuleHelperForTests ensuring that we have the right behavior for OVERWRITE (is given normally since the code is generic) and EXTEND (is bb specific)

msevestre · 2025-09-29T03:18:08Z

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

      private void performMerge()
      {
-         cacheBuilders(x => x.Reactions, _reactions);
+         var mergedReactions = mergeReactions(ReactionAndMergeBehaviors);


remove when implemented in #2603

msevestre · 2025-09-29T03:18:12Z

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

      internal IReadOnlyList<(EventGroupBuildingBlock eventGroupBuildingBlock, MergeBehavior mergeBehavior)> EventGroupAndMergeBehaviors =>
         _simulationConfiguration.ModuleConfigurations.Where(x => x.Module.EventGroups != null).Select(x => (x.Module.EventGroups, x.MergeBehavior)).ToList();

+      internal IReadOnlyList<(ReactionBuildingBlock reactionBuildingBlock, MergeBehavior mergeBehavior)> ReactionAndMergeBehaviors =>


remove when implemented in #2603

msevestre · 2025-09-29T03:19:01Z

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

+               }
+               else
+               {
+                  extendReaction(current, incoming);


this is the method that we need to probably port over (the merge). The rest is done dynamically using the clone as I suggested (yes it also clones the formula)

Well, if I dont copy the formula as Im doing now. I get this over many tests.
Message:
System.Reflection.TargetInvocationException : Exception has been thrown by the target of an invocation.
----> System.NullReferenceException : Object reference not set to an instance of an object.

All related with the formula.
So maybe I`m not doing this as I should. Will mark it for conversation

msevestre · 2025-09-29T03:20:17Z

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

+
+      private static void extendReaction(ReactionBuilder target, ReactionBuilder incoming)
+      {
+         var byNameParam = target.Parameters.ToDictionary(x => x.Name, StringComparer.OrdinalIgnoreCase);


none of thos management of parmaeter stuff is required. We have the contaienr task that merge container structure already for us. It's being used in #2603

msevestre · 2025-09-29T03:21:22Z

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

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


I am not srue about this line to be honest.

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

msevestre · 2025-09-29T03:25:48Z

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

+
+      private static void extendReaction(ReactionBuilder target, ReactionBuilder incoming)
+      {
+         var byNameParam = target.Parameters.ToDictionary(x => x.Name, StringComparer.OrdinalIgnoreCase);


this feels a bit heavy considering we have a functiont aht returns null already
the code is already implemented for this in the merge taks so we can ignroe

public void AddOrReplaceInContainer(IContainer container, IEntity entityToAddOrReplace) { var existingChild = container.GetSingleChildByName(entityToAddOrReplace.Name); if (existingChild != null) container.RemoveChild(existingChild); container.Add(entityToAddOrReplace); }

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

msevestre · 2025-09-29T03:26:41Z

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

+
+         copy.Formula = incoming.Formula;
+
+         copy.CreateProcessRateParameter = incoming.CreateProcessRateParameter;


this should alreayd be taken over from the updatePropertiesFrom

Again, some failing tests if I exclude this in relation with the createprocessrate.

msevestre · 2025-09-29T03:27:03Z

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

+
+         copy.CreateProcessRateParameter = incoming.CreateProcessRateParameter;
+         copy.ProcessRateParameterPersistable = incoming.ProcessRateParameterPersistable;
+         copy.Id = incoming.Id;


that's the important bit. that is missing in #2612

rwmcintosh · 2026-01-02T14:28:26Z

@benjaperez1983 do you need this PR or branch anymore?

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

@benjaperez1983 close this one?

benjaperez1983 · 2026-01-21T13:43:27Z

@rwmcintosh closed, was waiting until the other branch (based on michael`s) was reviewed, but I believe we can close it now.

#2640 implement merge behavior on Reactions

2a76e21

benjaperez1983 marked this pull request as draft September 16, 2025 15:16

benjaperez1983 requested review from msevestre and rwmcintosh and removed request for msevestre September 16, 2025 16:13

msevestre reviewed Sep 16, 2025

View reviewed changes

msevestre requested changes Sep 16, 2025

View reviewed changes

tests/OSPSuite.Core.IntegrationTests/ModelConstructorIntegrationTests.cs Outdated Show resolved Hide resolved

benjaperez1983 added 2 commits September 17, 2025 09:35

#2640 comments

0f16bf3

#2640 restore test includes

61ca3da

#2640 changes for PR

348e14f

benjaperez1983 requested a review from msevestre September 17, 2025 07:56

benjaperez1983 added 2 commits September 17, 2025 10:36

#2640 add to builder source

dba0655

#2640 remove unwanted file

73d7cdd

msevestre requested changes Sep 29, 2025

View reviewed changes

#2640 fixes

b3b7e7e

benjaperez1983 mentioned this pull request Jan 2, 2026

2640 implement merge behavior for reactions after michaels change #2725

Merged

16 tasks

benjaperez1983 closed this Jan 21, 2026


		copy.Formula = incoming.Formula;

		copy.CreateProcessRateParameter = incoming.CreateProcessRateParameter;

Comments

Conversation

benjaperez1983 commented Sep 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Type of change

How Has This Been Tested?

Reviewer checklist

Screenshots (if appropriate):

Questions (if appropriate):

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

msevestre left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

benjaperez1983 commented Sep 17, 2025

Uh oh!

msevestre commented Sep 29, 2025

Uh oh!

msevestre left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

benjaperez1983 Oct 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rwmcintosh commented Jan 2, 2026

Uh oh!

rwmcintosh commented Jan 21, 2026

Uh oh!

benjaperez1983 commented Jan 21, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

benjaperez1983 commented Sep 16, 2025 •

edited

Loading

benjaperez1983 Oct 7, 2025 •

edited

Loading