search

Open-Systems-Pharmacology / PK-Sim

PK-Sim® is a comprehensive software tool for whole-body physiologically based pharmacokinetic modeling
Other
103 stars 50 forks source link

Expose all parameters in the simulation parameters view #2104

Open PavelBal opened 2 years ago

PavelBal commented 2 years ago

In the hierarchy view, show all parameters of the model.

msevestre commented 2 years ago

hum...we show all parameters except the one that were marked explicitly as hidden. What are you missing?

PavelBal commented 2 years ago

@StephanSchaller Maybe you can comment.

StephanSchaller commented 2 years ago

See https://github.com/Open-Systems-Pharmacology/PK-Sim/issues/2103

Yuri05 commented 2 years ago

Vote against this. s. https://github.com/Open-Systems-Pharmacology/PK-Sim/issues/2103#issuecomment-1067943947

PavelBal commented 2 years ago

Vote for.

StephanSchaller commented 2 years ago

Vote against this. s. https://github.com/Open-Systems-Pharmacology/PK-Sim/issues/2103#issuecomment-1067943947

Why?... do you have a reason (except that users might make mistakes)?. Like a technical hurdle or huge effort?

Yuri05 commented 2 years ago

The effort is low, the impact is high.

E.g. imagine enabling for editing constant parameters which may not be overwritten. And then you use "add all const" parameters in the sensitivity calculation. This will result in: a) huge wasting of time, because calcualtion of sensitivity of those parameters does not make any sense b) results are absolute garbage, because changing those parameters breaks the model

And so on. It just does not make any sense.

Yuri05 commented 2 years ago

interestingly, when setting ALL parameters to visible in v11, I cannot open a simulation anymore:

Application:
PK-Sim® 11.0.82

Sequence contains no elements

Stack trace:

at System.Linq.Enumerable.First[TSource](IEnumerable`1 source)
   at PKSim.Presentation.Mappers.DynamicGroupExpander.closestAppliedMolecule(IParameter parameter)
   at System.Linq.Lookup`2.Create[TSource](IEnumerable`1 source, Func`2 keySelector, Func`2 elementSelector, IEqualityComparer`1 comparer)
   at System.Linq.GroupedEnumerable`3.GetEnumerator()
   at System.Linq.Buffer`1..ctor(IEnumerable`1 source)
   at System.Linq.OrderedEnumerable`1.<GetEnumerator>d__1.MoveNext()
   at PKSim.Presentation.Mappers.DynamicGroupExpander.addDynamicParameterGroupNodeTo(ITreeNode node, String subGroupName, IEnumerable`1 allParameters, Func`2 keySelector, Func`2 keyAvailableFunc)
   at PKSim.Presentation.Mappers.DynamicGroupExpander.addDynamicParameterGroupNodeTo(ITreeNode node, String subGroupName, IEnumerable`1 allParameters, Func`2 keySelector)
   at PKSim.Presentation.Mappers.DynamicGroupExpander.addCompoundApplicationTo(ITreeNode node, IEnumerable`1 allParameters)
   at PKSim.Presentation.Mappers.DynamicGroupExpander.AddDynamicGroupNodesTo(ITreeNode node, IReadOnlyCollection`1 allParameters)
   at PKSim.Presentation.Services.ParameterGroupNodeCreator.<>c__DisplayClass5_0.<mapGroup>b__0(ITreeNode n)
   at OSPSuite.Utility.Extensions.EnumerableExtensions.Each[T](IEnumerable`1 list, Action`1 action)
   at PKSim.Presentation.Services.ParameterGroupNodeCreator.mapGroup(IGroup group, IReadOnlyCollection`1 allParameters, Func`2 mapper, Boolean addDynamicNode, Boolean removeEmptyGroups)
   at PKSim.Presentation.Services.ParameterGroupNodeCreator.MapFrom(IGroup group, IReadOnlyCollection`1 allParameters)
   at PKSim.Presentation.Presenters.Parameters.ParameterGroupsPresenter.<>c__DisplayClass43_0.<parameterGroupsFor>b__3(IGroup group)
   at System.Linq.Enumerable.WhereSelectEnumerableIterator`2.MoveNext()
   at System.Collections.Generic.List`1..ctor(IEnumerable`1 collection)
   at System.Linq.Enumerable.ToList[TSource](IEnumerable`1 source)
   at PKSim.Presentation.Presenters.Parameters.ParameterGroupsPresenter.parameterGroupsFor(IEnumerable`1 visibleParameters)
   at PKSim.Presentation.Presenters.Parameters.ParameterGroupsPresenter.defaultNodesFor(IEnumerable`1 parameters)
   at PKSim.Presentation.Presenters.Parameters.ParameterGroupsPresenter.initSimpleGroupView()
   at PKSim.Presentation.Presenters.Parameters.ParameterGroupsPresenter.set_ParameterGroupingMode(ParameterGroupingMode value)
   at PKSim.Presentation.Presenters.Parameters.ParameterGroupsPresenter.InitializeWith(IContainer container, IEnumerable`1 allParameters)
   at PKSim.Presentation.Presenters.Simulations.IndividualSimulationParametersPresenter.EditSimulation(IndividualSimulation simulation)
   at PKSim.Presentation.Presenters.Simulations.EditSimulationPresenter`4.<>c__DisplayClass1_0.<InitializeSubPresentersWith>b__0(TSubPresenter x)
   at OSPSuite.Utility.Extensions.EnumerableExtensions.Each[T](IEnumerable`1 list, Action`1 action)
   at PKSim.Presentation.Presenters.Simulations.EditSimulationPresenter`4.InitializeSubPresentersWith(TSimulation simulation)
   at OSPSuite.Presentation.Presenters.EditAnalyzablePresenter`4.Edit(TAnalyzable analyzable)
   at OSPSuite.Presentation.Presenters.SubjectPresenter`3.Edit(Object objectToEdit)
   at PKSim.Presentation.Core.SingleStartPresenterTask.StartForSubject[T](T subject)
   at PKSim.Presentation.Services.BuildingBlockTask.Edit(IPKSimBuildingBlock buildingBlockToEdit)
   at PKSim.Presentation.Presenters.Main.ExplorerPresenter`2.EditBuildingBlock(IPKSimBuildingBlock buildingBlock)
   at PKSim.Presentation.Presenters.Main.SimulationExplorerPresenter.NodeDoubleClicked(ITreeNode node)
   at OSPSuite.UI.Views.BaseExplorerView.<>c__DisplayClass16_0.<nodeDoubleClick>b__0()
   at OSPSuite.Utility.Extensions.ExceptionExtensions.DoWithinExceptionHandler(Object callerObject, Action actionToExecute)
Sulav1 commented 2 years ago

I would also vote for exposing all the parameters. There are certain parameters like 1) kcat interaction factor for transporter proteins 2) Organism > Lumen > Transcellular absorption sink condition etc. which are accessible in MOBI and via codes but not using PK-Sim. However, those parameters would be of interest for PBPK models of certain compounds.

msevestre commented 2 years ago

@Sulav1 That's why we need to list all parameters that can be of interest :)

Specifically look at https://github.com/Open-Systems-Pharmacology/PK-Sim/issues/2104#issuecomment-1068038711

Sulav1 commented 2 years ago

@msevestre Probably, such extended list might be possible. Probably @sfrechen can comment.

sfrechen commented 2 years ago

I actually would vote for exposing ALL parameters. Maybe via a setting in the PK-Sim configuration.

Yuri05 commented 2 years ago

I actually would vote for exposing ALL parameters.

As read only - to be discussed. As editable - never ever.

To show all currently hidden parameters (as read only) - maybe in Hierarchy mode? So as soon as the user switches into this mode - ALL parameters are shown and you see more or less what you see in MoBi now: grafik

Alternatively: define new "Super advanced" mode for this

sfrechen commented 2 years ago

As editable - never ever.

But that is exactly what is required in special cases. And there is no list of "most common ones"... It cannot be predicted what is needed at some time in some project...

Alternatively: define new "Super advanced" mode for this

Fine for me. Sounds appropriate for the way we work ;-)

PavelBal commented 2 years ago

Super advanced but editable.

sfrechen commented 2 years ago

Super advanced but editable.

Definitely editable. Everything else is useless...

msevestre commented 2 years ago

We have a bunch of parameters that are marked as read only. The assumptions in the software is that these parameters cannot be changed. We are using this to simplify many of our checks when exporting parameters, checking if a value has changed etc ...this would all have to be checked again and tested. I would therefore also be against allowing read-only params to be edited. This will have unexpected side effects...I can guarantee

Now , the solution is to remove the read-only flag on parameters that could potentially be changed and leave it only on those where changing it would definitely result in model corruption. Or we remove the read-only flag altogether, I don't care anymore at this point. But allowing read-only parameter to be edited in PKSim will result in a lot of pain

StephanSchaller commented 2 years ago

Call it "Expert" view.

Well I would agree to find a solution without causing pain, but I agree with Sebastian

But that is exactly what is required in special cases. And there is no list of "most common ones"... It cannot be predicted what is needed at some time in some project...

Also, we can handle MoBi, so we should be fine not to "destroy model integrity" ?

So let's find a convenient path forward for everyone :-)

msevestre commented 2 years ago

as I said, the only way is to remove readonly flag. I feel Like I am repeating myself more and more these days. So one last time 1-the solution is to remove the read-only flag on parameters that could potentially be changed and leave it only on those where changing it would definitely result in model corruption OR 2- we remove the read-only flag altogether,

Allowing editing of Read-only parameter will create bugs in the software. Snapshot will not be exported as expected etc.. Clear?

Yuri05 commented 2 years ago

ReadOnlyParameters.xlsx Attached is the list of all parameters which are currently read only in PK-Sim. I have splitted them into 3 groups:

StephanSchaller commented 2 years ago

I feel Like I am repeating myself more and more these days.

Ah well, I misunderstood and didn't get that "removing the read-only flag altogether" would already be the painless solution.

@Yuri05 , thanks for this overview. I would agree that changing yellow and blue parameters does not make much sense. I would still vote for removing the flag altogether as I still believe users should be able to judge this themselves.

  • Yellow highlighted parameters: should never ever be editable. Changing them will corrupt the model 100%.

Isn't FcRn concentration in container a parameter that only defines the initial condition based on an analytical solution for a steadystate and could also be allowed to be changed? For the rest I agree, but most people understand not to change formula-based (dependent) parameters.

  • Blue highlighted parameters: changing them will either corrupt the model with pretty high probability or does not make any sense

Here I could imagine a use-case where someone would want to evaluate the change in partitioning coefficient value Partition coefficient (intracellular/water) even if it violates the assumption of equal pls/tissue free concentrations but it would not destroy mass balance.

  • Parameters without highlighing: can be potentially set as editable. When changing them - good luck. But at least the model will not be corrupted (in the worst case - just badly parameterized)

Should interstitial|Volume not be marked yellow in this case? changing this would leat to total organ volume no longer matching the sum of subcompartment volumes?

So overall, we don't know what users might want to try and not allowing this at all makes no sense to me if removing the read-only property can be a setting that has to be explicitly selected by the user and is not a default.

Yuri05 commented 2 years ago

I would still vote for removing the flag altogether as I still believe users should be able to judge this themselves.

I will not give my vote for this option. S. the discussion above.

Isn't FcRn concentration in container a parameter that only defines the initial condition based on an analytical solution for a steadystate and could also be allowed to be changed?

No, it's the current concetration of FcRn in EndogenousIgg. <Tissue Organ>|<Compartment>|FcRn Concentration => EndogenousIgG|<Compartment>|FcRn|Concentration

Here I could imagine a use-case where someone would want to evaluate the change in partitioning coefficient value Partition coefficient (intracellular/water) even if it violates the assumption of equal pls/tissue free concentrations but it would not destroy mass balance.

Agree, would not destroy mass balance. Would remove the connection to K_cell_pls, but that's than in the responsibility of a user.

Should interstitial|Volume not be marked yellow in this case? changing this would leat to total organ volume no longer matching the sum of subcompartment volumes?

Yep, you are right. The table was just a first draft and must be reviewed for sure.

StephanSchaller commented 2 years ago

The table was just a first draft and must be reviewed for sure.

Nice to have but not needed if we remove the flag altogether ;-)

Yuri05 commented 2 years ago

Nice to have but not needed if we remove the flag altogether ;-)

If 😄 After invading southern Greece and receiving the submission of other key city-states, Philip II of Macedon turned his attention to Sparta and asked menacingly whether he should come as friend or foe. The reply was "Neither." Losing patience, he sent the message: If I invade Laconia, I shall turn you out. The Spartan ephors again replied with a single word: If.