search

Open-Systems-Pharmacology / OSPSuite-R

R package for the OSPSuite
https://www.open-systems-pharmacology.org/OSPSuite-R/
Other
29 stars 12 forks source link

Error when computing steadystates with named simulations #1429

Closed Felixmil closed 5 months ago

Felixmil commented 5 months ago

1383 introduced the possibility to give simulation names and get identicaly named simulation results.

However, since getSteadyState function relies on simulation unique ids, using named simulations leads to the following error:

Error in validateIsOfType(simulationResults, "SimulationResults") : 
  project$runScenarios: argument 'simulationResults' is of type 'NULL', but expected 'SimulationResults'!
PavelBal commented 5 months ago

getSteadyState had no issue with this; it is the code that tried to process the results of getSteadyState, assuming that the names of the returned list are the names of the simulations input list. However, only runSimulations got extended by this feature (which still gives me shivers).

To be consistent with runSimulations, we could also extend getSteadyState (as you did in this PR). However, please also adjust the documentation of the function and add an entry to NEWS (similarly as how it was done for runSimulations).

And while you are at it, please add a check (for runSimulations and getSteadyState) for names uniqueness!

Felixmil commented 5 months ago

Well, the bug was occurring within getSteadyState becaused it relied on simId that was indeed impacted by #1383.

I looked for "@param simulations" string and it appears I also need to extend .storeSimulationState() and I will do it in #1430

I don't see the point adding a NEWS entry for a fix/modification of a feature that is not yet released.

Good suggestion for uniqueness check. Thanks !

PavelBal commented 5 months ago

I don't see the point adding a NEWS entry for a fix/modification of a feature that is not yet released.

Yeah ok, steady-state is new for the upcoming release, you are right. But the documentation of the function must be adjusted (that simulations can be a named list).

Felixmil commented 5 months ago

Will certainly do ! I'll also mention it must be unique.