Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fixes #2439 Misleading info in global POP PK Analysis if no parameter… #2459

Merged
merged 2 commits into from
Dec 7, 2022
Merged

Fixes #2439 Misleading info in global POP PK Analysis if no parameter… #2459

merged 2 commits into from
Dec 7, 2022

Conversation

rwmcintosh
Copy link
Member

…s can be calculated

@rwmcintosh rwmcintosh requested a review from msevestre December 5, 2022 20:09
@rwmcintosh rwmcintosh self-assigned this Dec 5, 2022
rwmcintosh
rwmcintosh commented Dec 5, 2022
View reviewed changes
src/PKSim.Presentation/Presenters/Simulations/GlobalPKAnalysisPresenter.cs
@@ -164,6 +165,20 @@ public bool HasParameters()
return false;
}

public bool CanCalculateGlobalPK()
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I ran resharper to auto-format so there's some noise in this PR. The gist is we need to calculate when a simulation contains only multiple IV applications of all compounds. If there's only single IV, oral, or a mix we want to show the global panel.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it this how it was doen for individual simulation?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for individual, the global is calculated on demand, so if it can calculate a global, the presenter just indicates that the datatable has rows and the view should show the panel.

For populations, you might have to re-run if the results are from an older version because the global would not have been calculated, and will not be calculated just because you show it.

msevestre
msevestre requested changes Dec 6, 2022
View reviewed changes
src/PKSim.Core/Services/PKAnalysesTask.cs Outdated
quantityPKList.Add(quantityPKParameterFor(globalIndividualPKParameterCache, pKParameter, group.Key));
});
});
aPKAnalysis?.AllPKParameters.GroupBy(moleculeNameFrom).Each(group => { group.Each(pKParameter => { quantityPKList.Add(quantityPKParameterFor(globalIndividualPKParameterCache, pKParameter, group.Key)); }); });
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks very hard to read. Why is this all on one line suddenly?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah, shoot. That's a Resharper.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

src/PKSim.Core/Services/PKAnalysesTask.cs
@@ -648,20 +637,20 @@ private bool isMultipleOral(Simulation simulation, Compound compound)
return allSchemaItems.Any() && allSchemaItems.All(isOral);
}

private static bool isOral(ISchemaItem schemaItem)
private static bool isOral(SchemaItem schemaItem)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why this chnage from Interface to not interface?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I implemented a bool property on SchemaItem that matches 'IsOral' and 'IsUserDefined' (IsIV) and like those ones, I did not pull them up to interface. I'm pretty sure all those conversions are local in this class.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am wondering if we need the ISchemaItem at all. Another day

src/PKSim.Presentation/Presenters/Simulations/GlobalPKAnalysisPresenter.cs
@@ -164,6 +165,20 @@ public bool HasParameters()
return false;
}

public bool CanCalculateGlobalPK()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it this how it was doen for individual simulation?

src/PKSim.Presentation/Presenters/Simulations/GlobalPKAnalysisPresenter.cs Outdated
});
}

private bool isMultipleIV(IReadOnlyList<SchemaItem> schemaItems)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the other hand, those small methods can really be on one line. no { } etc... makes sense to me

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's similar. In Individual =>
_view.GlobalPKVisible = _globalPKAnalysisPresenter.HasParameters();

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

src/PKSim.Presentation/Presenters/Simulations/PopulationPKAnalysisPresenter.cs Outdated
{
_pkAnalysesTask = pkAnalysesTask;
_exportTask = exportTask;
_populationPKAnalysisToDTOMapper = populationPKAnalysisToDTOMapper;
_globalPKAnalysisPresenter = globalPKAnalysisPresenter;
AddSubPresenters(_globalPKAnalysisPresenter);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hum...this should go in the parent presenter and be gone here I think

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't that what this diff is showing? It's been removed here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I see what you mean, the other lines referencing that object can go up too

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

src/PKSim.Presentation/Presenters/Simulations/PopulationPKAnalysisPresenter.cs
@@ -67,7 +65,9 @@ public void CalculatePKAnalyses(IPopulationDataCollector populationDataCollector
var simulation = populationDataCollector.DowncastTo<PopulationSimulation>();

//Calculate first global PK that should always be updated
_globalPKAnalysisPresenter.CalculatePKAnalysis(new[] {simulation});
_globalPKAnalysisPresenter.CalculatePKAnalysis(new[] { simulation });
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not even sure this guy should be protected (_globalPKAnalysisPresenter)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess the changes for this would be limited to -> can we use the same conditions for the individual presenter. Right now the assumption is that when you open the view, you can calculate what you need so you only need to check if the data table has rows.

For populations, it's not an adequate test because you might be able to create the datatable with rows if you re-run the simulation. So, the multipleIV test was implemented.

Can we use the multipleIV test in both cases?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They're also different how they export to Excel, but I'm not sure why that is.

tests/PKSim.Tests/Presentation/GlobalPKAnalysisPresenterSpecs.cs Outdated
}


public class when_calculating_if_a_global_pk_analysis_is_possible_for_single_iv : context_for_can_calculate_global_pk_analysis
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

when = > When

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

msevestre
msevestre approved these changes Dec 7, 2022
View reviewed changes
src/PKSim.Core/Services/PKAnalysesTask.cs
@@ -648,20 +637,20 @@ private bool isMultipleOral(Simulation simulation, Compound compound)
return allSchemaItems.Any() && allSchemaItems.All(isOral);
}

private static bool isOral(ISchemaItem schemaItem)
private static bool isOral(SchemaItem schemaItem)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am wondering if we need the ISchemaItem at all. Another day

@msevestre msevestre merged commit 5c2fdd9 into develop Dec 7, 2022
@msevestre msevestre deleted the 2439_multiple_iv_never_show_global_pk branch December 7, 2022 17:18
msevestre added a commit that referenced this pull request Mar 20, 2023
@msevestre @rwmcintosh @Yuri05
No more psv (#2567)
1c2e8a6 
* Fixes #2439 Misleading info in global POP PK Analysis if no parameter… (#2459)

* Fixes #2439 Misleading info in global POP PK Analysis if no parameters can be calculated

* PR feedback

* Fixes #2469 show hidden entities (#2488)

* Fixes buil failing (#2489)

* Fixes #2495 failing tests (#2499)

* Fixes #2485 observed added mobi (#2500)

* Fixes #2484 bug when exporting pop simulation (#2503)

* Fixes #2275 #2279 #2277 by updating OSPSuite.Core (#2506)

* Fixes #2405 mapping lost after configure (#2507)

* Fixes #2255 resetting concentration issue (#2512)

* Fixes #2255 resetting concentration issue

* Small update to stay symetrical

* Update MoleculeExpressionTask.cs

* Modify the comment to reflect population

Co-authored-by: Robert McIntosh <[email protected]>

* update core (#2514)

* Prepare for v11_1 (#2515)

* Fixes #2516 Cannot run population simulations imported from pkml (#2517)

* Fixes #2516 Cannot run population simulations imported from pkml

* PR feedback

* Fixes #2518 11.1 Cannot show PK-Analysis in a population simulations … (#2520)

* Fixes #2518 11.1 Cannot show PK-Analysis in a population simulations imported from pkml

* PR feedback

* PR feedback

* Updated OSPSuite.Core and OSPSuite.BuildingBlockTemplates (#2524)

* Updated OSPSuite.Core and OSPSuite.BuildingBlockTemplates

* Updated OSPSuite.Core again

Co-authored-by: Yuri05 <[email protected]>

* Update documentation v11.1

* Fixes #2553 pksim changes molecule name (#2561)

* Fixes #2536 loading project from snapshot observed data (#2560)

* Fixes #2536 loading project from snapshot observed data

* Fixes #2536 loading project from snapshot observed data

* Fixes #2536 loading project from snapshot observed data

* Fixes #2558 project created with v10< do not export properly (#2559)

* Fixes #2563 notification weird column no image data (#2564)

* Try update core

* Update to latest core

* More work to make sure we can still load indiviudals

* Remove creation of PSV for simulation

* Fixes test project"

* Fixes test project"

* No more psv

---------

Co-authored-by: Robert McIntosh <[email protected]>
Co-authored-by: Juri Solodenko <[email protected]>
Co-authored-by: Yuri05 <[email protected]>
rwmcintosh added a commit that referenced this pull request Apr 3, 2023
@msevestre @rwmcintosh @Yuri05
No more psv (#2567)
eccb8b4 
* Fixes #2439 Misleading info in global POP PK Analysis if no parameter… (#2459)

* Fixes #2439 Misleading info in global POP PK Analysis if no parameters can be calculated

* PR feedback

* Fixes #2469 show hidden entities (#2488)

* Fixes buil failing (#2489)

* Fixes #2495 failing tests (#2499)

* Fixes #2485 observed added mobi (#2500)

* Fixes #2484 bug when exporting pop simulation (#2503)

* Fixes #2275 #2279 #2277 by updating OSPSuite.Core (#2506)

* Fixes #2405 mapping lost after configure (#2507)

* Fixes #2255 resetting concentration issue (#2512)

* Fixes #2255 resetting concentration issue

* Small update to stay symetrical

* Update MoleculeExpressionTask.cs

* Modify the comment to reflect population

Co-authored-by: Robert McIntosh <[email protected]>

* update core (#2514)

* Prepare for v11_1 (#2515)

* Fixes #2516 Cannot run population simulations imported from pkml (#2517)

* Fixes #2516 Cannot run population simulations imported from pkml

* PR feedback

* Fixes #2518 11.1 Cannot show PK-Analysis in a population simulations … (#2520)

* Fixes #2518 11.1 Cannot show PK-Analysis in a population simulations imported from pkml

* PR feedback

* PR feedback

* Updated OSPSuite.Core and OSPSuite.BuildingBlockTemplates (#2524)

* Updated OSPSuite.Core and OSPSuite.BuildingBlockTemplates

* Updated OSPSuite.Core again

Co-authored-by: Yuri05 <[email protected]>

* Update documentation v11.1

* Fixes #2553 pksim changes molecule name (#2561)

* Fixes #2536 loading project from snapshot observed data (#2560)

* Fixes #2536 loading project from snapshot observed data

* Fixes #2536 loading project from snapshot observed data

* Fixes #2536 loading project from snapshot observed data

* Fixes #2558 project created with v10< do not export properly (#2559)

* Fixes #2563 notification weird column no image data (#2564)

* Try update core

* Update to latest core

* More work to make sure we can still load indiviudals

* Remove creation of PSV for simulation

* Fixes test project"

* Fixes test project"

* No more psv

---------

Co-authored-by: Robert McIntosh <[email protected]>
Co-authored-by: Juri Solodenko <[email protected]>
Co-authored-by: Yuri05 <[email protected]>
msevestre added a commit that referenced this pull request Jun 23, 2023
@msevestre @rwmcintosh
@georgeDaskalakis @Yuri05
Reintegrate changes in develop (#2668)
3d4f47b 
* Fixes #2344 Implement "Parameter Value Export" for Expression Profiles BB in PK-Sim (#2442)

* use GetAllChildren of Individual (#2445)

* 2438 outptu mappings not cleared (#2444)

* waiting for Core PR to be accepted

* nugets updated

* updating nuget

* 2364 export expression profile domain object  (#2461)

* initial try

* temp nuget update

* adjusting mapper

* Fixes #2344 Implement "Parameter Value Export" for Expression Profiles BB in PK-Sim (#2442)

* use GetAllChildren of Individual (#2445)

* 2438 outptu mappings not cleared (#2444)

* waiting for Core PR to be accepted

* nugets updated

* updating nuget

* temp nuget update

* working export

* spacing

* updating packages

* do not map object path for  'ROOT'

* do not repeat formulae in the formula cache

* adding tests

* fixing test

* correction

* consolidate assets

* correcting nugets

* code review changes

* code review

* PR feedback

* update core version

* Appveyor builds turned on

* PR feedback

* pr feedback

Co-authored-by: Robert McIntosh <[email protected]>

* Fixes #2470 Refactor StartValues vs PathWithValueEntity (#2473)

* 2475 expression profile from pksim (#2476)

* #784_expression_profile_from_pksim

* updating with mapper

* Fixes #2475 Create expression profile from PK-sim

* add dimensions as resource for tests

* Add pkparameters for test project

* add sqlite db

* PR feedback

* Fixes #2480 individual serialization (#2494)

* work in progress

* work in progress

* Revise to map origin data as strings instead of keeping a common OriginData class

* Update core and mapping of origin data

* Testing the mapper

* testing the mapper

* revert an unintentional change

* PR Feedback

* Update core (#2502)

* Fixes #2508 Do not export calculation methods with Individual when there is only one possible value (#2510)

* Fixes #2508 Do not export calculation methods with Individual when there is only one possible value

* PR feedback

* PR feedback and update core

* update core

* update core osmoses (#2513)

* update core (#2528)

* Fixes update latest core (#2541)

* 2525 creating individual from MoBi (PK-Sim code changes) (#2526)



---------

Co-authored-by: Robert McIntosh <[email protected]>

* Fixes #2534 Imported expression profile is not populated from the PKS… (#2548)

* Fixes #2534 Imported expression profile is not populated from the PKSim Database

* PR Changes

* PR feedback

* update core

* Fixes #2552 Export of Expression profile to PKML - include initial conditions (#2554)

* Update PKSIm with latest changes (#2555)

* Fixes #2551 Use DevExpress skin to color grid cells (#2556)

* No more psv (#2567)

* Fixes #2439 Misleading info in global POP PK Analysis if no parameter… (#2459)

* Fixes #2439 Misleading info in global POP PK Analysis if no parameters can be calculated

* PR feedback

* Fixes #2469 show hidden entities (#2488)

* Fixes buil failing (#2489)

* Fixes #2495 failing tests (#2499)

* Fixes #2485 observed added mobi (#2500)

* Fixes #2484 bug when exporting pop simulation (#2503)

* Fixes #2275 #2279 #2277 by updating OSPSuite.Core (#2506)

* Fixes #2405 mapping lost after configure (#2507)

* Fixes #2255 resetting concentration issue (#2512)

* Fixes #2255 resetting concentration issue

* Small update to stay symetrical

* Update MoleculeExpressionTask.cs

* Modify the comment to reflect population

Co-authored-by: Robert McIntosh <[email protected]>

* update core (#2514)

* Prepare for v11_1 (#2515)

* Fixes #2516 Cannot run population simulations imported from pkml (#2517)

* Fixes #2516 Cannot run population simulations imported from pkml

* PR feedback

* Fixes #2518 11.1 Cannot show PK-Analysis in a population simulations … (#2520)

* Fixes #2518 11.1 Cannot show PK-Analysis in a population simulations imported from pkml

* PR feedback

* PR feedback

* Updated OSPSuite.Core and OSPSuite.BuildingBlockTemplates (#2524)

* Updated OSPSuite.Core and OSPSuite.BuildingBlockTemplates

* Updated OSPSuite.Core again

Co-authored-by: Yuri05 <[email protected]>

* Update documentation v11.1

* Fixes #2553 pksim changes molecule name (#2561)

* Fixes #2536 loading project from snapshot observed data (#2560)

* Fixes #2536 loading project from snapshot observed data

* Fixes #2536 loading project from snapshot observed data

* Fixes #2536 loading project from snapshot observed data

* Fixes #2558 project created with v10< do not export properly (#2559)

* Fixes #2563 notification weird column no image data (#2564)

* Try update core

* Update to latest core

* More work to make sure we can still load indiviudals

* Remove creation of PSV for simulation

* Fixes test project"

* Fixes test project"

* No more psv

---------

Co-authored-by: Robert McIntosh <[email protected]>
Co-authored-by: Juri Solodenko <[email protected]>
Co-authored-by: Yuri05 <[email protected]>

* Fixes #2570 Move IndividualToIndividualBuildingBlockMapper from presentation to core (#2572)

* 2569 use individualbuildingblock and module from core (#2571)

* WIP #2569 use Individual Building block and module from core

* Some local variable rename. Build configuration not used anymore

* Map also indiviudal and expression building block. Needs #2570 to compile

* More work on #2569 to make test pass again:

* Fixes #2569 use of individual building blocks

* Fixes #2569 use individual building block and module from core

* Fixes #2577 Exported expression profiles contains duplicate formulae (#2578)

* 2584 update to support moduleconfiguration (#2585)

* Fixes #2584 update to support module configuration

* Fixes #2584 update to support module configuration

* Make test pass again

* 2586 parameters not common for all species (#2587)

* Added views to DB

* DB - adjusted naming of view and columns

* Fixes #2586 Provide info which parameters are not common for all species

* Fixes #2586 Provide info which parameters are not common for all species

* review comments

* Add serializer for the new dummy type

---------

Co-authored-by: Yuri05 <[email protected]>

* Fixes #2589 only export species indepednent parameters to spatial str… (#2591)

* Fixes #2589 only export species indepednent parameters to spatial structure

* Update to real DLLs

* Fixes typo

* Remove unused action

* Fixes update core (#2593)

* Fixes update core

* Fixes weird definition

* Update latest core (#2594)

* update to core 12.0.170 (#2604)

* Fixes #2596 implement hepatic impairment disease state (#2597)

* WIP #2596 implement hepatic impairment disease state

* More work on #2596

* More work on #2596

* More work on #2592

* Fixes #2596 implement hepatic impairement disease sate

* Fixes #2596

* Fixes #2596

* Small update

* Fixes merge conflict

* 2615 overriding ref conc and applying hi should use the updated refconc (#2620)

* Some rename first

* Add test showing issue with #2615

* Update to algorithm to match specificaitons

* Move logic of disease state to own view

* Fixes #2615 overriding ref conc

* Update DiseaseStateDTO.cs

just a typo

* Fixes failing test

* Fixes #2615 overriding ref conc

---------

Co-authored-by: Robert McIntosh <[email protected]>

* Core updates (#2625)

* Update to 12.0.178

* update to 12.0.178

* Update core

* update core (#2629)

* Fixes #2621. Fixes #2622 (#2630)

* Fixes #2621. Fixes #2622

* Fixes #2621. Fixes #2622

* Fixes #2621

* update core (#2633)

* Fixes #2624. Fixes #2623 (#2637)

* Fixes #2638 cannot export HI to snapshot (#2639)

* Fixes #2638 cannot export HI to snapshot

* Update DiseaseState.cs

make sure we use old way of namespacing

* Update DiseaseStateMapper.cs

* Update DiseaseStateMapper.cs

* Fixes #2638. Use file scoped namespace

* Fixes #2638 by reverting to namespace

* 2632 errorvalue cannot be null parameter name source (#2641)

* WIP #2632

* Fixes #2632 error value cannot be null

* Also null for protocol mapper

* Fixes #2643 when to show global pk analysis (#2644)

* Fixes #2643 when to show global pk analysis

* Fixes #2643 when to show global pk analysis

* Could not let it go

* 2627 no expression profiles created when exporting a simulation from pk sim to mobi (#2645)

* Fixes #2627 no expression profiles created

* Fixes #2627 no expression profiles created

* Fixes type

* Fixes update latest core (#2648)

* 2640 export initial conditions for expressions as part of the expression profile (#2649)

* Fixes #2640 Export initial conditions for expressions as part of the expression profile

* Update core

* separate MoleculeBuilders by their presence and definition in the individual

* rename

* refactor enumerable

* PR feedback

* remove dead starter tests project

* core updated to 12.0.191. No diffbuilders with Dimension specified (#2652)

* 2654 fix failing tests (#2655)

* Fixes #2654 by removing support for version <7.0

* Fixes #2654 by removing support for version <7.0

* Fixes #2658 differences in simulations created by pk sim and mobi molecule types (#2659)

* Too many molecules in molecules list for some observers

* Do not change container observer type to Drug

* Fixes #2532 (#2636)

* WIP #2532

* fix using the wrong assertion

* Add default parameters to all molecules

* add parameter to actual proten

* Also add the parmaeter values

* add reference to tablews

* WIP #2532 ontogeny factor in expression profile

* WIP #2532 ontogeny factor

* WIP #2532 ontogeny factor

* Rename db field

* Make PMA only human dependent

* 2657 ontogeny factor is it dimensionless or fraction (#2662)

* Fixes #2657 ontogeny factor dimensionless

* Fixes #2657

* Fixes #2661 exception when using 2 pore simulations (#2663)

* Fixes #2660 remove estimated ga and use PMA instead (#2664)

* Fixes #2660 remove estimated ga and use PMA instead

* Actual real formula

* Fixes #2617 export to snapshot (#2665)

* undo changes in appveryor

* undo changes in appveryor

* Update to latest core

---------

Co-authored-by: Robert McIntosh <[email protected]>
Co-authored-by: georgeDaskalakis <[email protected]>
Co-authored-by: Juri Solodenko <[email protected]>
Co-authored-by: Yuri05 <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@msevestre msevestre msevestre approved these changes

Assignees

@rwmcintosh rwmcintosh

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@rwmcintosh @msevestre