Skip to content

Exact intervals#885

Open
lukovdm wants to merge 27 commits intostormchecker:masterfrom
lukovdm:exactIntervals
Open

Exact intervals#885
lukovdm wants to merge 27 commits intostormchecker:masterfrom
lukovdm:exactIntervals

Conversation

@lukovdm
Copy link
Contributor

@lukovdm lukovdm commented Mar 10, 2026

Add exact Intervals and bounded model checking of intervals

lukovdm added 5 commits March 11, 2026 13:42
…related components and added tests

- Updated AddUncertainty transformer to handle RationalNumber types, allowing for exact arithmetic with RationalInterval.
- Modified various helper functions and model checkers to accommodate RationalNumber and RationalInterval.
- Introduced new tests for RationalNumber scenarios in model checking and uncertainty transformations.
- Ensured compatibility with existing models while expanding functionality for uncertain models using RationalNumber.
@lukovdm lukovdm marked this pull request as ready for review March 11, 2026 16:50
@sjunges
Copy link
Contributor

sjunges commented Mar 11, 2026

This includes #832, which should thus be reviewed/merged first?

Copy link
Contributor

@tquatmann tquatmann left a comment

Choose a reason for hiding this comment

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

I haven't completed my review yet, but wanted to make sure that my comments don't get lost somehow after merging #832 :)

@lukovdm lukovdm requested review from sjunges and tquatmann March 20, 2026 08:07

#include "storm/modelchecker/results/ExplicitQualitativeCheckResult.h"

#include "storm/adapters/IntervalForward.h"
Copy link
Contributor

Choose a reason for hiding this comment

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

It feels a bit strange to use a forward in a cpp file? Or do i miss something?

.build())
.build());
this->addOption(storm::settings::OptionBuilder(moduleName, soundOptionName, false, "Sets whether to force sound model checking.").build());
this->addOption(storm::settings::OptionBuilder(moduleName, intervalOptionName, false, "Sets whether to enable interval model checking.").build());
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need this? What does this mean? Do we still need this with the recent changes by Tim regarding the ability to parse model files as an interval model based on introspection of the model file?

#include <optional>

#include "storm/adapters/IntervalAdapter.h"
#include "storm/adapters/IntervalForward.h"
Copy link
Contributor

Choose a reason for hiding this comment

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

This should not be necessary if we already include the adapter?

#include <boost/range/adaptor/reversed.hpp>
#include <boost/range/irange.hpp>
#include <functional>
#include <iostream>
Copy link
Contributor

Choose a reason for hiding this comment

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

this is a really expensive header (especially without PCH).. Why do we need it? If we really need it, does also work?

}
};
if (storm::solver::minimize(dir)) {
if (choices) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we maximize when the dir is minimize?

#include <type_traits>

#include "storm/adapters/IntervalAdapter.h"
#include "storm/adapters/IntervalForward.h"
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldnt be necessary?

method = MinMaxMethod::PolicyIteration;
} else {
STORM_LOG_WARN("The selected solution method " << toString(method) << " does not guarantee exact results.");
// STORM_LOG_WARN("The selected solution method " << toString(method) << " does not guarantee exact results.");
Copy link
Contributor

Choose a reason for hiding this comment

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

I dont think we should comment it out like this :-)

@sjunges
Copy link
Contributor

sjunges commented Mar 20, 2026

Great work! I have not checked the new finite-horizon model checking algorithm (yet).

Also, have you checked that we didnt miss (or recently introduce) an instance of std::is_same_v<ValueType, storm::Interval>? I also noted that we are not always consistently using constexpr in front of that, but that is maybe also not so important :)

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.

3 participants