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

Fixes #1672 Errors are not propagated on failed simulation run #1709

Merged
merged 4 commits into from
Sep 28, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions src/OSPSuite.Assets/UIConstants.cs
Original file line number Diff line number Diff line change
Expand Up @@ -1644,6 +1644,8 @@ public static string SimulationPKAnalysesFileDoesNotHaveTheExpectedFormat
}
}

public static string SimulationDidNotProduceResults = "Simulation did not produce results";

public static string DuplicatedIndividualResultsForId(int individualId) => $"Individual results for individual with id '{individualId}' were defined more than once!";

public static string DuplicatedPKParameterSensitivityFor(string id) => $"PKParameter sensitivity results for '{id}' were defined more than once!";
Expand Down
4 changes: 2 additions & 2 deletions src/OSPSuite.Core/Domain/Services/ConcurrencyManager.cs
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ public interface IConcurrencyManager
/// <typeparam name="TResult">Data produced by the worker function</typeparam>
/// <param name="data">List of data to consume by the workers</param>
/// <param name="func">
/// A function running on each worker on each piece of data.
/// A function running on each worker on each piece of data. Needs to throw exceptions if the <paramref name="func"/> is not successful
/// </param>
/// <param name="cancellationToken">Cancellation token to cancel the threads</param>
/// <param name="numberOfCoresToUse">Number of cores to use. Use null to take all cores</param>
Expand All @@ -56,7 +56,7 @@ int numberOfCoresToUse
/// <typeparam name="TData">Data type to consume by the worker function</typeparam>
/// <param name="data">List of data to consume by the workers</param>
/// <param name="action">
/// An action running on each worker on each piece of data.
/// An action running on each worker on each piece of data. Needs to throw exceptions if the <paramref name="action"/>is not successful
/// </param>
/// <param name="cancellationToken">Cancellation token to cancel the threads</param>
/// <param name="numberOfCoresToUse">Number of cores to use. Use null to take all cores</param>
Expand Down
19 changes: 16 additions & 3 deletions src/OSPSuite.Core/Domain/Services/SimModelBatch.cs
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
using System;
using System.Collections.Generic;
using System.Linq;
using OSPSuite.Assets;
using OSPSuite.Core.Domain.Data;
using OSPSuite.Core.Serialization.SimModel.Services;
using OSPSuite.SimModel;
Expand Down Expand Up @@ -79,12 +80,13 @@ public SimulationRunResults RunSimulation()
{
updateSimulationValues();
_simModelSimulation.RunSimulation();
return new SimulationRunResults(success: true, warnings: WarningsFrom(_simModelSimulation), results: getResults());
var hasResults = simulationHasResults(_simModelSimulation);

return createSimulationRunResults();
}
catch (Exception e)
{
return new SimulationRunResults(success: false, warnings: WarningsFrom(_simModelSimulation), results: new DataRepository(),
error: e.FullMessage());
return createSimulationRunResults(e.FullMessage());
}
finally
{
Expand All @@ -93,6 +95,17 @@ public SimulationRunResults RunSimulation()
}
}

SimulationRunResults createSimulationRunResults(string errorFromException = null)
{
var hasResults = simulationHasResults(_simModelSimulation);
var warnings = WarningsFrom(_simModelSimulation);
var error = errorFromException ?? (hasResults ? null : Error.SimulationDidNotProduceResults);

return new SimulationRunResults(success: error == null, warnings, getResults(), error);
}

private bool simulationHasResults(Simulation simModelSimulation) => simModelSimulation.AllValues.Any();

public void UpdateParameterValue(string parameterPath, double value) => _parameterValueCache[parameterPath] = value;

public void UpdateParameterValues(IReadOnlyList<double> parameterValues)
Expand Down
10 changes: 7 additions & 3 deletions src/OSPSuite.R/Domain/SimulationBatch.cs
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
using OSPSuite.Core.Domain.Services;
using OSPSuite.R.Extensions;
using OSPSuite.SimModel;
using OSPSuite.Utility.Exceptions;
using OSPSuite.Utility.Extensions;

namespace OSPSuite.R.Domain
Expand Down Expand Up @@ -148,12 +149,15 @@ public SimulationResults Run(SimulationBatchRunValues simulationBatchRunValues)
{
_simModelBatch.UpdateParameterValues(simulationBatchRunValues.Values);
_simModelBatch.UpdateInitialValues(simulationBatchRunValues.MoleculeValues);
var simulationResults = _simModelBatch.RunSimulation();
var simulationRunResults = _simModelBatch.RunSimulation();

if (!simulationRunResults.Success)
throw new OSPSuiteException(simulationRunResults.Error);
Copy link
Member Author

Choose a reason for hiding this comment

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

Unsuccessful run must be signaled by exception

Copy link
Member

Choose a reason for hiding this comment

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

yep


if (!_simulationBatchOptions.CalculateSensitivity)
return _simulationResultsCreator.CreateResultsFrom(simulationResults.Results);
return _simulationResultsCreator.CreateResultsFrom(simulationRunResults.Results);

return _simulationResultsCreator.CreateResultsWithSensitivitiesFrom(simulationResults.Results, _simModelBatch, _simulationBatchOptions.Parameters);
return _simulationResultsCreator.CreateResultsWithSensitivitiesFrom(simulationRunResults.Results, _simModelBatch, _simulationBatchOptions.Parameters);
}

protected virtual void Cleanup()
Expand Down
45 changes: 45 additions & 0 deletions tests/OSPSuite.R.Tests/Domain/SimulationBatchSpecs.cs
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
using OSPSuite.Core.Extensions;
using OSPSuite.R.Services;
using OSPSuite.SimModel;
using OSPSuite.Utility.Exceptions;
using OSPSuite.Utility.Extensions;

namespace OSPSuite.R.Domain
Expand All @@ -29,6 +30,50 @@ public override void GlobalContext()
}
}

public class When_running_a_batch_simulation_with_an_error : concern_for_SimulationBatch
{
private SimulationBatchOptions _simulationBatchOptions;
private SimulationResults _results;
private SimulationBatchRunValues _simulationBatchRunValues;

public override void GlobalContext()
{
base.GlobalContext();

// Force an error during simulation run
_simulation.SimulationSettings.Solver.MxStep = 3;
_simulationBatchOptions = new SimulationBatchOptions
{
VariableMolecules = new[]
{
new[] {"Organism", "Kidney", "Intracellular", "Caffeine"}.ToPathString()
},

VariableParameters = new[]
{
new[] {"Organism", "Liver", "Volume"}.ToPathString(),
new[] {"Organism", "Hematocrit"}.ToPathString(),
}
};
sut = _simulationBatchFactory.Create(_simulation, _simulationBatchOptions);
}

protected override void Because()
{
_simulationBatchRunValues = new SimulationBatchRunValues
{
InitialValues = new[] { 10.0 },
ParameterValues = new[] { 3.0, 0.53 }
};
}

[Observation]
public void should_throw_an_exception_during_run()
{
The.Action(() => sut.Run(_simulationBatchRunValues)).ShouldThrowAn<OSPSuiteException>();
}
}

public class When_running_a_batch_simulation_run : concern_for_SimulationBatch
{
private SimulationBatchOptions _simulationBatchOptions;
Expand Down
66 changes: 66 additions & 0 deletions tests/OSPSuite.R.Tests/Services/ConcurrentSimulationRunnerSpecs.cs
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
using OSPSuite.Core.Domain.Services;
using OSPSuite.Core.Extensions;
using OSPSuite.R.Domain;
using OSPSuite.Utility.Exceptions;

namespace OSPSuite.R.Services
{
Expand Down Expand Up @@ -87,6 +88,71 @@ public void should_complete_without_error()
}
}

public class When_running_a_batch_simulation_with_exception : concern_for_ConcurrentSimulationRunner
{
private ConcurrentRunSimulationBatch _concurrentRunSimulationBatch;
private ConcurrencyManagerResult<SimulationResults>[] _results;
private IModelCoreSimulation _simulation;
private readonly List<string> _ids = new List<string>();
private readonly List<SimulationBatchRunValues> _simulationBatchRunValues = new List<SimulationBatchRunValues>();

public override void GlobalContext()
{
base.GlobalContext();
_simulation = _simulationPersister.LoadSimulation(HelperForSpecs.DataFile("S1.pkml"));

_concurrentRunSimulationBatch = new ConcurrentRunSimulationBatch
(
_simulation,
new SimulationBatchOptions
{
VariableMolecules = new[]
{
new[] { "Organism", "Kidney", "Intracellular", "Caffeine" }.ToPathString()
},

VariableParameters = new[]
{
new[] { "Organism", "Liver", "Volume" }.ToPathString(),
new[] { "Organism", "Hematocrit" }.ToPathString(),
}
}
);

sut.AddSimulationBatch(_concurrentRunSimulationBatch);
}

protected override void Because()
{

_simulationBatchRunValues.Add(new SimulationBatchRunValues
{
InitialValues = new[] { 10.0 },
ParameterValues = new[] { 3.5, 0.53 }
});

// Force an error during simulation run because ParameterValues vector does not have enough values
_simulationBatchRunValues.Add(new SimulationBatchRunValues
{
InitialValues = new[] { 10.0 },
ParameterValues = new[] { 3.5 }
});

_concurrentRunSimulationBatch.AddSimulationBatchRunValues(_simulationBatchRunValues[0]);
_concurrentRunSimulationBatch.AddSimulationBatchRunValues(_simulationBatchRunValues[1]);

_ids.AddRange(_concurrentRunSimulationBatch.SimulationBatchRunValues.Select(x => x.Id));
_results = sut.RunConcurrently();
}

[Observation]
public void should_return_a_result_with_success_set_to_false()
{
_results.Count(x => x.Succeeded).ShouldBeEqualTo(1);
_results.Count(x => !x.Succeeded).ShouldBeEqualTo(1);
}
}

public class When_running_a_batch_simulation_run_concurrently : concern_for_ConcurrentSimulationRunner
{
private ConcurrentRunSimulationBatch _concurrentRunSimulationBatch;
Expand Down