-
Notifications
You must be signed in to change notification settings - Fork 84
Implemented support for verifying IDTMCs #808
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
base: master
Are you sure you want to change the base?
Implemented support for verifying IDTMCs #808
Conversation
…propositional model checking for interval DTMCs
tquatmann
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good progress! My comment regarding the interval optimization direction is related to #807 .
| solver->setHasUniqueSolution(false); | ||
| solver->setHasNoEndComponents(false); | ||
| solver->setLowerBound(storm::utility::zero<SolutionType>()); | ||
| solver->setUpperBound(storm::utility::one<SolutionType>()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The upper bound of 1 does not apply if we compute expected rewareds
| Environment const& env, CheckTask<storm::logic::UntilFormula, SolutionType> const& checkTask) { | ||
| storm::logic::UntilFormula const& pathFormula = checkTask.getFormula(); | ||
| if (storm::IsIntervalType<ValueType>) { | ||
| STORM_LOG_THROW(checkTask.isOptimizationDirectionSet(), storm::exceptions::InvalidPropertyException, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a bit odd that the optimization direction controls nature for IDTMCs (but not for IMDPs, it seems).
Suggestion: the interval optimization direction should be stored as an std::optional<storm::OptimizationDirection> in the Checktask (and then checked e.g. here). That forces an API user to set it explicitly.
When creating a checktask from a formula, the interval optimization direction can be derived automatically (following PRISM behaviour).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Of course, this also means that we have to read the IntervalOptimizationDirection from the SolveGoal (somewhere in the Helper).
| Environment const& env, CheckTask<storm::logic::EventuallyFormula, SolutionType> const& checkTask) { | ||
| storm::logic::EventuallyFormula const& eventuallyFormula = checkTask.getFormula(); | ||
| if (storm::IsIntervalType<ValueType>) { | ||
| STORM_LOG_THROW(checkTask.isOptimizationDirectionSet(), storm::exceptions::InvalidPropertyException, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See above
…uncertainty should be resolved by nature. This flag is required as soon as interval models are being checked.
src/storm-pars/modelchecker/region/SparseDtmcParameterLiftingModelChecker.cpp
Show resolved
Hide resolved
| @@ -1,5 +1,7 @@ | |||
| #pragma once | |||
|
|
|||
| #include "storm/adapters/IntervalAdapter.h" | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible to keep the include only in the cpp file? This avoids costly includes of carl in header files.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With the current design (ValueType precision; stored by value), this is not possible, because ValueType needs to be complete at the point where ConstantsComparator<ValueType> is instantiated. However, as an alternative, we could change it to std::shared_ptr<const ValueType> precision, and use the precision variable by dereferencing the pointer in the implementation (ConstantsComparator.cpp). This would avoid requiring ValueType to be complete in the header. If that is acceptable, then I can change the code accordingly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's change that to
| #include "storm/adapters/IntervalAdapter.h" | |
| #include "storm/adapters/IntervalForward" |
The pointer solution would yield a runtime overhead
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Incorporating this change results in the following error while executing the tests locally on my machine (same as before adding the include for IntervalAdapter.h):
error: ‘storm::utility::ConstantsComparator<ValueType>::precision’ has incomplete type.
tquatmann
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the contribution! Looks good, I just have some nitpicks :)
src/test/storm/modelchecker/prctl/mdp/RobustMdpPrctlModelCheckerTest.cpp
Outdated
Show resolved
Hide resolved
src/storm-pars/modelchecker/region/SparseDtmcParameterLiftingModelChecker.h
Outdated
Show resolved
Hide resolved
| @@ -1,5 +1,7 @@ | |||
| #pragma once | |||
|
|
|||
| #include "storm/adapters/IntervalAdapter.h" | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's change that to
| #include "storm/adapters/IntervalAdapter.h" | |
| #include "storm/adapters/IntervalForward" |
The pointer solution would yield a runtime overhead
| } | ||
|
|
||
| return result; | ||
| // Solve the corresponding system of equations. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We actually need to check the requirements. Something like
auto req = solver->getRequirements(env);
if (!computeReward) {
req.clearbounds();
}
STORM_LOG_THROW(!req.hasEnabledCriticalRequirement(), storm::exceptions::UncheckedRequirementException,
"Solver requirements " + req.getEnabledRequirementsAsString() + " not checked.");
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Temporarily fixed in dee5cad. I hope this is okay for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should throw already at this point. If the solver requires an upper bound and we ignore that requirement, it will fail or (even worse) terminate with a wrong result.
On another note: We should have negative rewards in mind. This means that 0 is not necessarily a lower bound. (I know there are still some parts that assume non-negative rewards).
Is there an issue with my suggestion?
|
What does it even mean that we use an interval as precision in the constants comparator? |
|
The following patch should handle the includes for ConstantsComparator. |
|
I can also push the changes to this PR if you want. |
Intuitively, I would have expected a behaviour like this, where we interpret the precision bounds as tolerances on the interval bounds: If this is not the intended semantics, then maybe we need to discuss this more thoroughly. |
That would be great. Thank you! About the failing build: is this due to an error in my implementation, or is it caused by the pipeline itself? I might be missing something, but I could not infer this from the error messages. |
|
I have merged the recent changes from master. This should fix the build issue on Alpine. |
|
@plindnercs so you use two precisions, one for the lower bound and one for the upper bound? Is this a hypothetical use case, or a real one? |
Yes, you can see it that way. Right now, it is only a hypothetical use case as far as I know. At least the implementation that I contributed does not make use of this directly. |
I'd be in favour of using the most obvious semantics for now, until something else is required. To me, "most obvious semantics" would be to have Edit: I realise that this, again, requires the |
…ainty resolution mode.
tquatmann
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Just some final remarks.
I also pushed a change to the storm::api::createTask interface. In my opinion, this should explicitly ask for an uncertainty resolution mode when dealing with interval models.
| } | ||
|
|
||
| return result; | ||
| // Solve the corresponding system of equations. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should throw already at this point. If the solver requires an upper bound and we ignore that requirement, it will fail or (even worse) terminate with a wrong result.
On another note: We should have negative rewards in mind. This means that 0 is not necessarily a lower bound. (I know there are still some parts that assume non-negative rewards).
Is there an issue with my suggestion?
| } else if (uncertaintyResolutionModeString == "cooperative") { | ||
| return UncertaintyResolutionModeSetting::Cooperative; | ||
| } else if (uncertaintyResolutionModeString == "both") { | ||
| STORM_LOG_ERROR("Uncertainty resolution mode 'both' not yet supported."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
STORM_LOG_ASSERT(false ...
with the validator above, this should not be reachable. It's better to just fail for now.
| uint64_t& numIterations, SolutionType const& precision, | ||
| const std::function<SolverStatus(const SolverStatus&)>& iterationCallback, | ||
| MultiplicationStyle mult, | ||
| const UncertaintyResolutionMode& uncertaintyResolutionMode) const { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For consistency: UncertaintyResolutionMode const&
| return false; | ||
| } | ||
| case UncertaintyResolutionMode::Unset: | ||
| STORM_LOG_WARN("Uncertainty resolution mode not set properly, assuming to resolve uncertainty in a robust fashion."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
STORM_LOG_THROW(false, ...
I fully second this. In particular, I would not even allow instantiating the constants comparator with intervals... |
I think the |
This feature extension includes IDTMC support to verify the following properties:
Note:
Pmin,Pmax,RminandRmaxare flipped compared to PRISM.minmax:methodis not manually set tovi, the current implementation prints a warning and defaults to robust value iteration. This is somewhat related to issue Default selected method not supported (dd's) #264. Note that the warning currently appears twice as theminmax:methodis checked in two places.I'm happy to receive any kind of feedback!