Support for interval-based transitions in storm#409
Support for interval-based transitions in storm#409sjunges merged 46 commits intostormchecker:masterfrom
Conversation
|
@volkm The tests currently fail due to It looks like this is an issue on the systems based on the debug basesystem. Could it be that they have a slightly outdated carl version? Do you build from tag or from head there? |
|
Yes, the pre-built images use carl-storm:stable and not the latest head. I will update it and then trigger a rerun. |
|
stormchecker/carl-storm#21 must be merged first |
* Moved implementation into the cpp file * avoid using exceptions for "expected" control flow * revise the implementation so that we don't need an expression evaluator for intervals
Co-authored-by: Tim Quatmann <tim.quatmann@cs.rwth-aachen.de>
Co-authored-by: Tim Quatmann <tim.quatmann@cs.rwth-aachen.de>
Co-authored-by: Tim Quatmann <tim.quatmann@cs.rwth-aachen.de>
…erTest.cpp Co-authored-by: Tim Quatmann <tim.quatmann@cs.rwth-aachen.de>
Co-authored-by: Tim Quatmann <tim.quatmann@cs.rwth-aachen.de>
|
@volkm @tquatmann I believe this is ready to be reviewed. I would value two pairs of eyes given the large number of essential files being touched. |
tquatmann
left a comment
There was a problem hiding this comment.
LGTM (besides the minor issues I pointed out)
Co-authored-by: Tim Quatmann <tim.quatmann@cs.rwth-aachen.de>
Co-authored-by: Tim Quatmann <tim.quatmann@cs.rwth-aachen.de>
Co-authored-by: Tim Quatmann <tim.quatmann@cs.rwth-aachen.de>
volkm
left a comment
There was a problem hiding this comment.
In general, it looks good to me. Thank you for all the work, in particular addingstorm::Interval and SolutionType in so many places.
I have two general comments:
- I would avoid the
if interval { throw ..} else { }and just useif interval { throw} .... That way the else part does not have to be indented and less lines are changed. But this is my personal opinion. - In a couple of places we still use
assert(..)instead ofSTORM_LOG_ASSERT(..). But maybe this is more a general issue which could be addressed in a separate issue.
Co-authored-by: Matthias Volk <volkm@users.noreply.github.com>
Co-authored-by: Matthias Volk <volkm@users.noreply.github.com>
Co-authored-by: Matthias Volk <volkm@users.noreply.github.com>
Co-authored-by: Matthias Volk <volkm@users.noreply.github.com>
Co-authored-by: Matthias Volk <volkm@users.noreply.github.com>
Co-authored-by: Matthias Volk <volkm@users.noreply.github.com>
Co-authored-by: Matthias Volk <volkm@users.noreply.github.com>
Co-authored-by: Matthias Volk <volkm@users.noreply.github.com>
Co-authored-by: Matthias Volk <volkm@users.noreply.github.com>
Co-authored-by: Matthias Volk <volkm@users.noreply.github.com>
|
@volkm I tried to remove the else parts, however, they are explicitly necessary right now to make it compile. I therefore ignore your suggestion :-) |
|
Once this passes all tests, I plan to squash and merge. Thanks for all the feedback. |
In one instance the issue was just because there was still a dangling But I am also fine with ignoring the comments. |
That was why the CI failed. But fixing that does not make the code compile. Note that these are |
|
You are fully right and now I understand the issue. Thanks for the explanation. |
volkm
left a comment
There was a problem hiding this comment.
All my comments have been addressed. We can merge it.
In particular, it requires
Things that I will currently not support. I track these in this issue: #469
Other things that I do not support and that I do not see happening right now