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

Add bindings for quantitative POMDP analysis #125

Merged
merged 6 commits into from
Nov 1, 2023

Conversation

AlexBork
Copy link
Contributor

Introduction of bindings in accordance with the reworked quantitative POMDP analysis in Storm 1.8.
This is largely based on the work @TheGreatfpmK and @randriu did for the Storm-PAYNT POMDP integration (SAYNT).

@volkm
Copy link
Contributor

volkm commented Jun 29, 2023

Depends on moves-rwth/storm#396

Comment on lines 18 to 21
print(result.lower_bound)
print(result.upper_bound)
print(result.induced_mc_from_scheduler)
print(result.cutoff_schedulers[0])
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we replace the print statements by assert? Then we can actually test the correctness.

@TheGreatfpmK
Copy link
Contributor

Hi everyone, can we work on getting this merged? moves-rwth/storm#396 is already merged and we updated PAYNT to work with Storm 1.8 so it would be nice to have SAYNT fully operational with updated Storm/StormPy/PAYNT.

@sjunges
Copy link
Contributor

sjunges commented Oct 31, 2023

@AlexBork can you resolve the remark by @volkm ?

That should also retrigger the tests :-)

@AlexBork
Copy link
Contributor Author

Sorry, I think I somehow missed this completely. I adjusted the tests and merged the latest state of main, so let's see if this works.

@sjunges
Copy link
Contributor

sjunges commented Oct 31, 2023

Many thanks for your prompt response :-)

@volkm
Copy link
Contributor

volkm commented Oct 31, 2023

Thanks for integrating my comments.
The stable test currently fails. This is in principle not an issue, just increase the required Storm version. Looking at the failure it might be possible to fix the include though.

@AlexBork
Copy link
Contributor Author

If I understand that correctly, the test runs using the stable branch of Storm. The include is already fixed in the latest Storm version, so isn't it necessary to release a new stable version containing the fix? Or can I change the required Storm version to a certain commit? If so, can you give me a pointer how this is done?

@volkm
Copy link
Contributor

volkm commented Oct 31, 2023

Ah sorry, the error is in Storm and not stormpy. You need to increase the required Storm version here and set it to 1.8.2.

@sjunges sjunges merged commit c010e02 into moves-rwth:master Nov 1, 2023
9 checks passed
@TheGreatfpmK
Copy link
Contributor

Thanks everyone for a quick resolution of this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants