Feature/valuations - #424
Conversation
|
@volkm this wont compile unless I start merging the other PRs in Storm, but if you could have a look whether there is something major that you want changed, then we could avoid having the main thing nto compiling for too long? |
volkm
left a comment
volkm
left a comment
There was a problem hiding this comment.
Overall looks good to me. Mostly some smaller comments.
On a higher level, I was wondering whether it is needed to define some bindings in the C++ part and then add some extension in __init__.py? Would it be possible to define the binding only in the C++ part and add the type check there as well? This would make it easier if the code is not spread over two locations.
| storm::storage::BitVector bv = v.getBooleanValues(var); | ||
| std::vector<bool> result(bv.size(), false); | ||
| for (uint64_t index : bv) { | ||
| result[index] = true; | ||
| } | ||
| return result; |
There was a problem hiding this comment.
Should this maybe be part of Storm to convert from/to std::vector<bool>?
There was a problem hiding this comment.
For the C++ api, I dont see an advantage in doing that, but for the python api (where types are hidden) I think this is important to avoid bugs.
There was a problem hiding this comment.
Sorry, I meant whether we should add a dedicated method BitVector::toVector in Storm which returns a std::vector<bool>. This method could then be used in this binding.
The idea would be that such a dedicated toVector method could also be useful in other places.
There was a problem hiding this comment.
I havent done that yet. Can do that :-)
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>
| # Extend class Valuations | ||
| # get_value, get_values_states, write_value, get_boolean_values_states_as_bitvector, | ||
| # get_integer_bigint_value/write_integer_bigint_value, and get_double_value/get_double_values_states/ | ||
| # write_double_value (including their type checks) are implemented directly in src/storage/valuation.cpp. |
volkm
left a comment
volkm
left a comment
There was a problem hiding this comment.
Thanks for the changes, in particular moving the Python code to the C++ side.
Looks good to me now.
|
CI seems to improve, only one example is failing. Seems good to have additional checks now. |
2 participants
Match stormchecker/storm#937, requires stormchecker/storm#949 also to compile.