Fixes #2721 Path of a parameter of a Passive Transport cannot be resolved when "Create process rate" is enabled by rwmcintosh · Pull Request #2722 · Open-Systems-Pharmacology/OSPSuite.Core

rwmcintosh · 2025-12-15T20:45:34Z

Description

The path resolver can resolve paths that start with the container name, but the process rate parameter creation will not create the correct path adjustment for the process rate parameter.

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

…lved when "Create process rate" is enabled

Copilot

Pull request overview

This PR fixes a bug where path resolution failed for parameters in Passive Transport when "Create process rate" is enabled. The issue occurred because the process rate parameter creation did not correctly adjust path references when the path started with the container (process) name itself.

Key changes:

Enhanced path adjustment logic to handle paths that start with the process/container name
Added unit test coverage for the specific case where a path starts with the process name
Refactored the path adjustment method for better clarity and maintainability

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File	Description
`src/OSPSuite.Core/Domain/Mappers/ProcessRateParameterCreator.cs`	Enhanced `adjustRelativePath` method to handle paths starting with the process name by prepending two parent container references, allowing proper resolution when paths reference parameters within the process container itself
`tests/OSPSuite.Core.Tests/Domain/ProcessRateParameterCreatorSpecs.cs`	Added test case for path starting with process name ("Reaction", "C") to verify it correctly adjusts to include two parent container references

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

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

msevestre · 2025-12-16T02:03:40Z

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

         _kinetic.AddObjectPath(_formulaUsablePathFU);
         _formulaUsablePathBW = new FormulaUsablePath("Organism", "BW").WithAlias("BW");
         _kinetic.AddObjectPath(_formulaUsablePathBW);
+         _formulaUsablePathC = new FormulaUsablePath("Reaction", "C").WithAlias("C");


I don;t understand. What's this absolute path "Reaction"?

oh that' sthe name of the reaction defined below. This is super confusiong. Can we set the name higer up

I don't understand why the path look like this
why would the path in the process rate parmaeter be "P1|C". Should not that be "..|C"? or sthg like this
It feels like it;s absolute relative to the parent container. Do we have sthg like this anyewhere else?

Yeah we could make the new path ..|C. It would also work rather than keeping the name of the process. I think I like that better too.

Do we have sthg like this anyewhere else?

IDK how often it's used. In ObjectPath.Resolve we have this

//We have an absolute Path from the ref entity if (string.Equals(firstEntry, refEntity.Name)) usePath.RemoveAt(0);

This removes the name if it matches the reference name. This has been working for some time already. But this is why it works when building the simulation, but not when adding the ProcessRate

msevestre

a bit confused with the use cas.e Implementation is fine

msevestre · 2025-12-16T02:05:57Z

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

         _kinetic.AddObjectPath(_formulaUsablePathFU);
         _formulaUsablePathBW = new FormulaUsablePath("Organism", "BW").WithAlias("BW");
         _kinetic.AddObjectPath(_formulaUsablePathBW);
+         _formulaUsablePathC = new FormulaUsablePath("Reaction", "C").WithAlias("C");


oh that' sthe name of the reaction defined below. This is super confusiong. Can we set the name higer up

I don't understand why the path look like this
why would the path in the process rate parmaeter be "P1|C". Should not that be "..|C"? or sthg like this
It feels like it;s absolute relative to the parent container. Do we have sthg like this anyewhere else?

rwmcintosh · 2025-12-16T13:04:03Z

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

+         // to compensate. That's because a container name can be ignored when a path is resolved from within that container
+         if (objectPath.Count == 2 && string.Equals(objectPath[0], processName))
+         {
+            objectPath.AddAtFront(new ObjectPath(ObjectPath.PARENT_CONTAINER, ObjectPath.PARENT_CONTAINER));


@Yuri05 what do you think? should we make this case ..|..|ProcessName|ParameterName or remove the process name and go with ..|ParameterName

..|ParameterName would be better for sure (and process name invariant)

Well, yes, but in this case the process name is fixed because this is the ProcessRate parameter for the process.

But if we use the ..|ParameterName then it is the same as the case where the original path was only ParameterName

Copilot

Pull request overview

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

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

Copilot · 2025-12-16T14:39:57Z

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

-               objectPath.AddAtFront(ObjectPath.PARENT_CONTAINER);
-         }
-      }
+      private void addAdditionalParentReference(IFormula formula, string processName) => formula.ObjectPaths.Each(x => adjustRelativePath(x, processName));


The method uses an expression body with a lambda instead of a traditional method body. While this is syntactically valid, using an expression body for a method that only calls another method with side effects (Each with adjustRelativePath) reduces readability. Consider using a traditional method body with a foreach loop for better clarity, especially since the operation modifies the paths rather than returning a value.

Suggested change

private void addAdditionalParentReference(IFormula formula, string processName) => formula.ObjectPaths.Each(x => adjustRelativePath(x, processName));

private void addAdditionalParentReference(IFormula formula, string processName)

{

foreach (var objectPath in formula.ObjectPaths)

{

adjustRelativePath(objectPath, processName);

}

}

Copilot · 2025-12-16T14:39:58Z

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

-            return true;
+         // if the path starts with the  process name then we need to replace the name with ".."
+         // to compensate. That's because a container name can be ignored when a path is resolved from within that container
+         if (objectPath.Count == 2 && string.Equals(objectPath[0], processName))


The condition only handles paths with exactly 2 elements that start with the process name. If a path has 3 or more elements and starts with the process name (e.g., ["Reaction", "X", "Y"]), it won't be adjusted correctly. The condition should check if the path starts with the process name regardless of the total count. Consider changing the condition to just check if the first element matches the process name without the count restriction.

Suggested change

if (objectPath.Count == 2 && string.Equals(objectPath[0], processName))

if (string.Equals(objectPath[0], processName))

Copilot · 2025-12-16T14:39:58Z

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


-         if (objectPath[0] == ObjectPath.PARENT_CONTAINER)
-            return true;
+         // if the path starts with the  process name then we need to replace the name with ".."


There is an extra space in the comment: "the process" should be "the process".

Suggested change

// if the path starts with the process name then we need to replace the name with ".."

// if the path starts with the process name then we need to replace the name with ".."

Copilot · 2025-12-16T14:39:58Z

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

      private FormulaUsablePath _formulaUsablePathBW;
      private SimulationBuilder _simulationBuilder;
+      private FormulaUsablePath _formulaUsablePathC;
+      private readonly string _processBuilderName = "Reaction";


The field is declared as readonly but initialized inline instead of in the constructor. While this is valid C# syntax, it's inconsistent with the typical pattern in test classes where such fields are initialized in the Context method. Consider initializing this in the Context method for consistency, or use a const if the value never changes.

Suggested change

private readonly string _processBuilderName = "Reaction";

private const string _processBuilderName = "Reaction";

Copilot

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

Fixes #2721 Path of a parameter of a Passive Transport cannot be reso…

99a98ec

…lved when "Create process rate" is enabled

rwmcintosh requested review from Yuri05, benjaperez1983 and msevestre December 15, 2025 20:45

rwmcintosh self-assigned this Dec 15, 2025

Copilot AI review requested due to automatic review settings December 15, 2025 20:45

Copilot started reviewing on behalf of rwmcintosh December 15, 2025 20:46 View session

Copilot AI reviewed Dec 15, 2025

View reviewed changes

src/OSPSuite.Core/Domain/Mappers/ProcessRateParameterCreator.cs Outdated Show resolved Hide resolved

msevestre reviewed Dec 16, 2025

View reviewed changes

link names

f37938f

rwmcintosh commented Dec 16, 2025

View reviewed changes

PR feedback

0b71a56

rwmcintosh requested review from Copilot and msevestre December 16, 2025 14:28

Copilot started reviewing on behalf of rwmcintosh December 16, 2025 14:36 View session

Copilot AI reviewed Dec 16, 2025

View reviewed changes

Yuri05 approved these changes Dec 16, 2025

View reviewed changes

rwmcintosh merged commit 7455ba7 into V13 Dec 16, 2025
7 checks passed

rwmcintosh deleted the 2721-path-of-a-parameter-of-a-passive-transport-cannot-be-resolved-when-create-process-rate-is-enabled branch December 16, 2025 16:16

PavelBal mentioned this pull request Feb 2, 2026

Regression after #2722 - path of a reaction incorrectly resolved #2762

Closed

	if (objectPath.Count == 2 && string.Equals(objectPath[0], processName))
	if (string.Equals(objectPath[0], processName))

	// if the path starts with the process name then we need to replace the name with ".."
	// if the path starts with the process name then we need to replace the name with ".."

	private readonly string _processBuilderName = "Reaction";
	private const string _processBuilderName = "Reaction";

Comments

Conversation

rwmcintosh commented Dec 15, 2025

Description

Type of change

How Has This Been Tested?

Reviewer checklist

Screenshots (if appropriate):

Questions (if appropriate):

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

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!

Choose a reason for hiding this comment

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!

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!

Copilot AI Dec 16, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 16, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 16, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 16, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants