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.

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

ConcurrentSimulationRunner: Error when one simulation fails #968

Closed
PavelBal opened this issue Apr 26, 2021 · 5 comments · Fixed by #970
Closed

ConcurrentSimulationRunner: Error when one simulation fails #968

PavelBal opened this issue Apr 26, 2021 · 5 comments · Fixed by #970

Comments

@PavelBal
Copy link
Member

If one of the simulations passed to the ConcurrentSimulationRunner fails, the run is terminated with the error produced by that simulation:

results <- runSimulationsConcurrently(c(sim, sim2))
Error in rClr::clrCall(simulationRunner, "RunConcurrently") : 
  Type:    OSPSuite.Utility.Exceptions.OSPSuiteException
Message: Simulation run failed: some variables became negative. There are different possible reasons for this:

  - Solver tolerances are too high. Please reduce the absolute and relative tolerances by one order of magnitude (e.g. from 1E-9 to 1E-10) and restart the simulation.
  - Some variables which are allowed to be negative were defined as non-negative.
  - Model is not properly established (e.g. the kinetic should be k*[A] but was defined as k*[B] etc.)

The following variables became negative :
...

I propose to throw an error that explicitly states the id of the failed simulation, e.g.

Message: Simulation Id asdsadsad: Simulation run failed: some variables became negative. There are different possible reasons for this:

@PavelBal
Copy link
Member Author

@msevestre @abdelr

@msevestre
Copy link
Member

@PavelBal Agreed. This could be part of the result object as well somehow (eg. error object with the error and a status, success or fail)

@PavelBal
Copy link
Member Author

IMO, it would be sufficient just to terminate with an error giving a hint which simulation did not succeed. Otherwise the caller has to check if all runs terminated successfully, and, well, if a simulation failed then the user clearly has to do something about it manually.

@abdelr
Copy link
Member

abdelr commented Apr 27, 2021

I agree it is best to run the simulations that were able to run and return the error only for the simulation with problems while still providing the results for the rest. We could in the future have some flag to terminate pending simulations when any of them fails if you want but I see situations where it will still be good to have the rest of the results. I have been working on this today. I am moving this logic to the ConcurrencyManager since it should be available for any class using concurrency. I think it will be ready soon. I just need to make sure there is no loose end. @PavelBal, good catch by the way.

@abdelr
Copy link
Member

abdelr commented Apr 27, 2021

@PavelBal I would strongly recommend against terminating earlier. It is true the caller has to fix the simulations before keep going but now imagine you want to run 10 simulations and 4 of them are failing. If we terminate on first error you would need to run four failing runs before you are ready to go. While by just waiting theoretically a bit more, you should have had complete information so you could fix the problems at once and in the second run you should be able to check your results. I also see other options where you would like to still see the results of the remaining simulations.

@abdelr abdelr linked a pull request Apr 27, 2021 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants