include pk properties for population simulations #2227

abdelr · 2022-05-10T12:18:54Z

Fixes #2021 (include pk properties for population simulations)

abdelr · 2022-05-10T12:21:11Z

@PavelBal: Just created a PR for easier revision but still in progress. Currently, it uses the mean for the population global PK. It also expects the user to select the [peripheral] venous blood plasma as an output. Still missing the second option for the normal PK.

msevestre · 2022-05-10T12:53:48Z

FYI @Open-Systems-Pharmacology/suite-core-developers

This should not be merged before we release v11

abdelr · 2022-05-10T13:39:41Z

FYI @Open-Systems-Pharmacology/suite-core-developers

This should not be merged before we release v11

I do not think it will be ready before we release. Still pending some logic plus tests

Yuri05 · 2022-05-10T14:06:09Z

I do not think it will be ready before we release

Well.. who knows 😉

abdelr · 2022-05-12T07:05:20Z

I do not think it will be ready before we release

Well.. who knows 😉

Well, I hope the release does not get that much delayed lol

abdelr · 2022-05-12T07:08:17Z

@msevestre, @Yuri05 and @georgeDaskalakis: Tests are green now but there are missing tests on the new logic. You could though start taking a look on the PR if you like so we do not end up with a huge set of changes. If at some point you do review the PR then I will start to add new incremental features by new PR on these branch instead of committing directly to it so it will be easier to review. For now, the new tests I will commit directly unless you tell me me not to do it since you are reviewing so I start already with the new PRs policy.

msevestre · 2022-05-12T11:33:14Z

src/PKSim.Core/Mappers/PopulationPKAnalysisToDataTableMapper.cs

@@ -37,7 +37,7 @@ private void addAnalysisToTable(PopulationPKAnalysis analysis, DataTable dataTab

      private string getCurveName(PopulationPKAnalysis analysis)
      {
-         return analysis.CurveData.Caption;
+         return analysis.CurveData.Caption + analysis.ExtraDescription;


use string interpolation instead of concatenating strings. Not a big deal here but just something to simply do all the time so that strings are not allocated for nothing

msevestre · 2022-05-12T11:35:14Z

src/PKSim.Core/Model/Extensions/DataColumnExtensions.cs

-            return true;
-
-         return simulation.IsAnImplementationOf<PopulationSimulation>();
+         return individualSimulationContainsColumn(dataColumn, simulation);


Don't quite understand this change. Seems to be equivalent to me?

For what I have seen in the code, when we receive a population simulation the DataInfo.Origin is not properly set. My thought was that for the case of populations it is not meaningful (or at least in this case) but as we discussed, it makes sense to see why it has not been set. So the change is indeed very similar but the only difference is that for population simulations it will not check the origin at all. Before it would return false always. I will still check why the origin is not been set.

msevestre · 2022-05-12T11:35:36Z

src/PKSim.Core/Model/IndividualSimulation.cs


-      public IndividualSimulation()
+      public IndividualSimulation() : base()


calling base() is not required. Remove

Why not? There is some code in the base constructor that needs to be executed still. More specifically
_compoundPKCache = new Cache<string, CompoundPK>(x => x.CompoundName, x => new CompoundPK());

Because the base() constructor is always called....or am I missing something?

msevestre · 2022-05-12T11:36:45Z

src/PKSim.Core/Model/IndividualSimulation.cs

@@ -113,5 +70,23 @@ public override void AcceptVisitor(IVisitor visitor)
         if(HasResults)
            DataRepository.AcceptVisitor(visitor);
      }
+
+      public override DataColumn PeripheralVenousBloodColumn(string compoundName)


Inline all of this for readability. They are just forward methods

msevestre · 2022-05-12T11:37:12Z

src/PKSim.Core/Model/IndividualSimulation.cs

      }

      public virtual IEnumerable<SimulationTimeProfileChart> TimeProfileAnalyses => Analyses.OfType<SimulationTimeProfileChart>();

-      public void ClearPKCache()


@abdelr It would be good to comment a bit on PR. I have no idea why this is removed. This has nothing to do with pk properties for simulations

It was moved to the base class so population simulation can also use it.

msevestre · 2022-05-12T11:37:42Z

src/PKSim.Core/Model/PopulationSimulation.cs

@@ -24,7 +24,7 @@ public class PopulationSimulation : Simulation, IPopulationDataCollector, IAdvan

      public virtual AgingData AgingData { get; }

-      public PopulationSimulation()
+      public PopulationSimulation() : base()


remove base. This is always called

Fair enough

msevestre · 2022-05-12T11:39:16Z

src/PKSim.Core/Model/PopulationSimulation.cs

@@ -279,5 +279,65 @@ public override void UpdateFromOriginalSimulation(Simulation originalSimulation)
         var sourcePopSimulation = originalSimulation as PopulationSimulation;
         sourcePopSimulation?.AdvancedParameters.Each(x => AddAdvancedParameter(x, generateRandomValues: true));
      }
+
+      private DataColumn aggregateDataColumns(IEnumerable<DataColumn> columns)


Move this outside of the simulation. This is clearly some kind of an extension methods. We have quite a few to calculate "aggregates" stuff already

msevestre · 2022-05-12T11:39:40Z

src/PKSim.Core/Model/PopulationSimulation.cs

+
+      private DataColumn aggregateDataColumns(IEnumerable<DataColumn> columns)
+      {
+         var column = columns.First();


this will crash if there is no items. Clearly you are suspecting that this might be null see line below

What does first mean? We need to comment this type of code BTW.
Do not comment the obvious, but if it is not obvious, comment

msevestre · 2022-05-12T12:05:40Z

src/PKSim.Core/Model/PopulationSimulation.cs

+         return aggregateDataColumns(drugColumnFor(CoreConstants.Organ.LUMEN, CoreConstants.Observer.FABS_ORAL, CoreConstants.Observer.FABS_ORAL, compoundName));
+      }
+
+      private IEnumerable<DataColumn> drugColumnFor(string organ, string compartment, string columnName, string compoundName)


What is columnName? Also compoundName is not used

Good catch... compoundName should be used

msevestre · 2022-05-12T12:08:33Z

src/PKSim.Core/Model/Simulation.cs

@@ -19,6 +20,25 @@ public abstract class Simulation : PKSimBuildingBlock, ISimulation, IWithChartTe
      private readonly List<ISimulationAnalysis> _allSimulationAnalyses = new List<ISimulationAnalysis>();
      private SimulationProperties _properties;
      private SimulationResults _results;
+      private readonly ICache<string, CompoundPK> _compoundPKCache;
+
+      public virtual DataColumn PeripheralVenousBloodColumn(string compoundName)


I think this should be abstract. But I am actually not sure that this belongs here at all. Why are we implementing something specific for just a few outputs? Does not make much sense to me

It was already defined at IndividualSimulation and although you are right, it makes some sense since we are using this specific combination ([peripheral] blood plasma) a lot. It could be on a service though but keep in mind that it is extracted differently at Population/Individual Simulation. Also, it was not abstract since there are more implementations of Simulation and there it is not required but again, makes sense that it is abstract here and only on the concrete implementations we decide whether it returns something or not.

msevestre · 2022-05-12T12:08:57Z

src/PKSim.Core/Model/Simulation.cs

      }

+      private double? compoundPKValue(string compoundName, Func<CompoundPK, double?> pkValueFunc)


Why was this moved to the base class?

So it is used by both Individual/Population Simulation. There could be one class between Simulation and Population/Individual Simulation but it sounds like an overshooting to me.

msevestre · 2022-05-12T12:13:16Z

src/PKSim.Core/Services/PKAnalysesTask.cs

-         var allColumns = timeProfileChartData.Panes.SelectMany(x => x.Curves).Select(x =>
-               new {curveData = x, column = columnFor(x, populationDataCollector)})
-            .Where(c => c.column != null)
+         var allColumns = timeProfileChartData.Panes.SelectMany(x => x.Curves).SelectMany(x =>


I don't understand this change.

It refers to range results. Before range results only contained a single result by now it is calculated at both ends so the 5 and 95 % for example instead of 5% only.

msevestre · 2022-05-12T12:14:33Z

src/PKSim.Core/Services/PopulationPKAnalysis.cs


-      public PopulationPKAnalysis(CurveData<TimeProfileXValue, TimeProfileYValue> curveData, PKAnalysis pkAnalysis)
+      public PopulationPKAnalysis(CurveData<TimeProfileXValue, TimeProfileYValue> curveData, PKAnalysis pkAnalysis, string extraDescription = "")


string,empty

Compiler complains, it needs to be a compile time constant

msevestre · 2022-05-12T12:14:38Z

src/PKSim.Core/Services/PKAnalysesTask.cs

+
+         return new[] 
+         { 
+            new DataColumn("", curveData.YAxis.Dimension, baseGrid)


string,empty

msevestre

I need to understand what is going on here. I do not see in the #2021 issue some of the stuff matching the implementation and therefore I have no idea why this was changed

msevestre · 2022-05-12T12:20:38Z

oK i see where the request to move stuff comes from

Enable calculation of missing PK-Parameters. For parameters that require additional simulation runs (AUC_ratio, C_max_ratio, bioavailability), show a warning message to the user that this will require calculation of the whole population and might take much time.

msevestre · 2022-05-12T12:22:11Z

All in all, I think this goes in the right direction. I am hesitant of putting specific curves selections just for the sake of being used in one service somewhere. Maybe this belongs to the service instead. The aggregate methods implementation looks like we have it somewhere already. If not, we have code that manipulates array so this probably belongs here. (just to follow the overall implementation)

* Filter body weight dependent rows from population PK values * Using values instead of names to filter * rewriting for easier reading * rephrasing for better reading * No need for the two methods anymore

PavelBal

No remarks from user perspective any more.

Plase create a feature request for including "Global" PK-Parameters into the box-plot/scatter plot.

* UI changes * Without aggregation * Working with loooots of code duplication :( * Logic implemented using stored PKValues per individual * Removing last code duplication and unused error variable * Using real actual administration mode * simplifying names * Misc changes * Testing float matrix * Testing aggregate function * Revert "simplifying names" This reverts commit f3158d5. * Making names the same for individual and populations * Binding to an unique DTO * Fixing test * Making sure data is sorted * Refactoring upper/lower suffixes * Renaming pks * Some code beautifying and a more robust cast to PopulationSimulation * Adding tooltips * Changes from code review * partial commit before rebasing * Show message when no pk values * Toogle components depending on data * Using built in function * Bind on tab changed to refresh to current data * Refactoring logic from ChartsDataCreator * Calculate bioavailability * some misc changes * Moving logic from presenter * refactoring * missing file from last commit * Renaming xtraTabPages * Adding icons & undoing a bug from last commit * Rephrasing label a bit. * Showing label "Values calculated for the mean population curve" * Adding comment to explain why the need for rebinding

PavelBal · 2022-07-26T12:30:10Z

@abdelr I thought this was merged already?

msevestre · 2022-07-26T12:32:17Z

Let me check this one last time

msevestre · 2022-07-26T13:17:00Z

There was a bunch of comments that were not addressed and for some reason, the documentation file is being reverted. I'll have to spend some time on this. Also the build is failing so some tests are not ok....

msevestre · 2022-07-28T22:36:50Z

@PavelBal The tests are failing in this PR ...

I'll need to investigate why

msevestre · 2022-07-28T22:49:00Z

@PavelBal
Is this supposed to look like this?

msevestre · 2022-07-28T22:50:36Z

I guess it's only for old simulation where F_Abs was not available. But still..this is really ugly

PavelBal · 2022-07-29T08:17:48Z

I guess it's only for old simulation where F_Abs was not available. But still..this is really ugly

Yes this will only be shown when having an older simulation... all new simulations will show the parameters right away.

msevestre · 2022-07-29T15:05:43Z

ok. it's not because it's visible once that it should not be shown properly

…be done to simplify

msevestre · 2022-07-29T17:14:57Z

ok I have it

and

msevestre

@PavelBal @Yuri05 @StephanSchaller
The new behavior at the presenter level is absolutely not tested. I could move the whole code without having any issue with tests not working. I do not have time to add tests but in the future, we need to ensure that we test the behavior that we are adding,. This is our only way of survival with such a small team

msevestre · 2022-07-29T17:31:28Z

src/PKSim.Assets/PKSimConstants.cs

@@ -2629,6 +2631,10 @@ public static class PKAnalysis
         public static readonly string Unit = UI.Unit;
         public static readonly string Description = "Description";
         public static readonly string Warning = "Warning";
+         public static readonly string OnCurves = "On Curves";


@abdelr @PavelBal This constants were defined as per request...but not USED! So the hard coded strings were still used in the code behing

msevestre · 2022-07-29T17:32:27Z

src/PKSim.Core/Services/PKAnalysesTask.cs

+      /// <returns></returns>
+      PKAnalysis CreatePKAnalysisFromValues(PKValues pkValues, Simulation simulation, Compound compound);
+
+      IReadOnlyList<PopulationPKAnalysis> AggregatePKAnalysis(Simulation populationDataCollector, IEnumerable<QuantityPKParameter> pkParameters, IEnumerable<StatisticalAggregation> selectedStatistics, string captionPrefix);


I have no idea what this captionPrefix means. But I did not have time to investigate. Probably the name of the output but hard to tell

msevestre · 2022-07-29T17:33:10Z

src/PKSim.Presentation/Presenters/Charts/EditTimeProfileAnalysisChartPresenter.cs

@@ -183,7 +183,7 @@ private void showAnalysis()
      private void calculatePKAnalysis()
      {
         var chartData = CreateChartData();
-         _pkAnalysisPresenter.CalculatePKAnalysis(PopulationDataCollector, chartData);
+         _pkAnalysisPresenter.CalculatePKAnalyses(PopulationDataCollector, chartData, PopulationAnalysisChart.PopulationAnalysis);


I have rewritten this completely so that the PKCalculation was done ion pkAnalysisPresenter and not in this presenter.
TELL DON'T ASK.

msevestre · 2022-07-29T17:36:35Z

I will merge as soon as the build is running

abdelr requested a review from PavelBal May 10, 2022 12:18

abdelr self-assigned this May 10, 2022

abdelr marked this pull request as draft May 10, 2022 12:19

abdelr requested review from georgeDaskalakis, msevestre and Yuri05 May 12, 2022 07:08

msevestre reviewed May 12, 2022

View reviewed changes

msevestre requested changes May 12, 2022

View reviewed changes

abdelr force-pushed the 2021_Include_PK_Properties_for_Population_Simulations branch from 35513c8 to 9fc74f9 Compare May 20, 2022 06:59

abdelr added 7 commits July 13, 2022 17:51

Misc changes plus some extra tests

3fc69d6

Fixing test: using correct compound name

418b60e

Using aggregation functions already defined

c2b0fbd

making column selection methods abstract

827e8fc

Filter body weight dependent rows from population PK values (#2238)

9e82abc

* Filter body weight dependent rows from population PK values * Using values instead of names to filter * rewriting for easier reading * rephrasing for better reading * No need for the two methods anymore

default outputs on population simulation

8b89c68

removing action since it is the same now for both

fc64d08

abdelr force-pushed the 2021_Include_PK_Properties_for_Population_Simulations branch from e1e5c3e to fc64d08 Compare July 13, 2022 15:51

PavelBal approved these changes Jul 18, 2022

View reviewed changes

abdelr mentioned this pull request Jul 18, 2022

including "Global" PK-Parameters into the box-plot/scatter plot #2260

Closed

Update documentation

0241785

Make test run again

a604be0

msevestre added 2 commits July 29, 2022 11:06

Fixes alignment issues

d5fb9b0

Fixes remaining bugs and simplify code a bit. Still loads that could …

ba74a81

…be done to simplify

msevestre added 2 commits July 29, 2022 13:30

Simplify even more

37fd60b

Simplify even more

52b45a2

msevestre reviewed Jul 29, 2022

View reviewed changes

msevestre merged commit 27d6496 into develop Jul 29, 2022

msevestre deleted the 2021_Include_PK_Properties_for_Population_Simulations branch July 29, 2022 17:46


		public IndividualSimulation()
		public IndividualSimulation() : base()

		}

		private double? compoundPKValue(string compoundName, Func<CompoundPK, double?> pkValueFunc)


		public PopulationPKAnalysis(CurveData<TimeProfileXValue, TimeProfileYValue> curveData, PKAnalysis pkAnalysis)
		public PopulationPKAnalysis(CurveData<TimeProfileXValue, TimeProfileYValue> curveData, PKAnalysis pkAnalysis, string extraDescription = "")

include pk properties for population simulations #2227

include pk properties for population simulations #2227

Conversation

abdelr commented May 10, 2022 • edited Loading

abdelr commented May 10, 2022

msevestre commented May 10, 2022

abdelr commented May 10, 2022

Yuri05 commented May 10, 2022

abdelr commented May 12, 2022

abdelr commented May 12, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msevestre left a comment

Choose a reason for hiding this comment

msevestre commented May 12, 2022 • edited Loading

msevestre commented May 12, 2022

PavelBal left a comment

Choose a reason for hiding this comment

PavelBal commented Jul 26, 2022

msevestre commented Jul 26, 2022

msevestre commented Jul 26, 2022

msevestre commented Jul 28, 2022

msevestre commented Jul 28, 2022

msevestre commented Jul 28, 2022

PavelBal commented Jul 29, 2022

msevestre commented Jul 29, 2022

msevestre commented Jul 29, 2022

msevestre left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msevestre commented Jul 29, 2022

abdelr commented May 10, 2022 •

edited

Loading

msevestre commented May 12, 2022 •

edited

Loading