Changes to Storm for revised belief exploration - #1028
Conversation
| std::optional<TimeBound> BoundedUntilFormula::getUpperBoundAsOptionalTimeBound(unsigned i) const { | ||
| if (hasUpperBound(i)) { | ||
| return upperBound.at(i).get(); | ||
| } else { |
There was a problem hiding this comment.
Why not just return upperBound.at(i) rather than the case split (we would need to move from boost to std then, but that would be good anyways.
There was a problem hiding this comment.
PR #1030 addressed the change, so I will also fix this here :)
| @@ -0,0 +1,23 @@ | |||
| #pragma once | |||
There was a problem hiding this comment.
What happens here if InputType == OutputType? Why do we ever want those instantiations?
What do we do if we go from doubles to rational numbers to ensure that probabilities still sum to one?
There was a problem hiding this comment.
InputType == OutputType i somthing we should never need. I can add a static assertion that those are never generated.
I indeed did not consider the sum issue.
I cannot think of anything to address this properly.
As background: I want to use this transformation to transform a rational to a double POMDP to apply the preprocessing (checking of underlying MDP there) without needing to re-parse the input model.
For now, this is also not essential, so I am considering just throwing it out. Any thoughts?
There was a problem hiding this comment.
- Input=Output: We are declaring them right now, right? That was what promted this in the first place.
- I also understand the to-double use case, it is also something we sometimes do in the parametric setting.
- We could renormalize afterwards for the sum to make sense.
Maybe rename to "toDouble" that supports going from exact rationals to doubles fixes parts of my concerns. :-)
There was a problem hiding this comment.
Wow, I somehow completely overlooked that I added the Input=Output declarations :D
Will remove those.
| #include <string> | ||
| #include <vector> | ||
|
|
||
| #include "storm/models/sparse/Model.h" |
There was a problem hiding this comment.
Some simple test cases for this would be good.
2 participants
This PR contains changes to Storm outside Storm-POMDP for the revised belief exploration implementation, see PR #1004.