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

Nested soft constraint support #178

Merged
merged 4 commits into from
Feb 22, 2023

Conversation

alwilson
Copy link
Contributor

This addresses #176 by first separating each soft constraint into its own Implies soft constraint while building RandInfo. The condition for the Implies constraint is built up by keeping a stack of the if_then and implies conditions while visiting constraints and combining those with And expressions.

Then in the RandSetNodeBuilder a soft_phase flag has been added to prevent nested soft constraints from being built during the hard constraints. A new soft constraints build phase is added to specifically build soft and nested soft constraints.

There's a TODO in rand_info_builder.py under visit_constraint_soft about how a 'priority' attribute is being added to the Implies constraint built up for nested soft constraints so that they can be sorted alongside other soft constraints as well as make it possible
to check for soft vs hard constraints in the add_constraints function.

            # TODO Is it okay to add priority attr to root Constraint so
            #      add_constraint can detect them and randomize can sort
            #      soft constraints more easily?
            soft_implies = ConstraintImpliesModel(and_cond, [c])
            soft_implies.priority = c.priority
            self._active_randset.add_constraint(soft_implies)

I should reread the docs, but the soft constraints section might need to be changed.

Separate soft from hard constraints while building RandInfo.
Do this by building up a stack of the conditions behind each
'if/else/then' and 'implies' scope and generate an 'implies'
on the 'and' of those conditions for each soft constraint.

Also remove the soft constraint from those constraint_l after
doing so.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
Now that nested soft constraints are recreated as individual, soft
constraints the hard constraint building needs to skip them and
soft constraints need to be built with the soft flag enabled.
This happens both in the rand_set_node_builder and back in
randomizer when it constructs the list of python and Boolector
constraint tuples. The randomizer call should be picking up
cached objects, but still needs the flag to not return None
for the object. Unless there's a different way to do so?
When randsets combine they were call add_constraint on nested soft
constraints that had been added using add_soft_constraint to bypass
the ConstraintSoftModel check. Since the 'priority' attribute was
already being added to the base constraint in nested constraints
to make sorting soft constraints possible, I changed the add_constraint
check to test for that as well and removed the add_soft_constraint
function.
@alwilson
Copy link
Contributor Author

@mballance The wording in the docs mentions soft constraints being limited to expressions and nothing about nested constraints not working, so I don't think the docs need to be updated. Unless an example of nested soft constraints would be useful in the docs, then I could add something relevant.

The soft constraint applies to a single expression, as shown above. Soft constraints are disabled if they conflict with another hard constraint declared in the class or introduced as an inline constraint.

I'm not sure about the inline constraint bit. Inline constraints are randomize_with constraints, correct? I ran a test on master and the soft constraint still shows up in the debug print and appears to be constraining as expected. I don't recall what SV does here either, although I guess most of the time you would want hard constraints in that scenario.

@mballance
Copy link
Member

@alwilson, this looks great! I'm happy with the implementation, and it looks like all relevant corner cases are covered.
I'll go ahead and merge this. Please confirm whether you're okay with me listing your user name in the ChangeLog file.
Congrats on your first merged PR to the project!

@mballance mballance merged commit de14431 into fvutils:master Feb 22, 2023
@alwilson
Copy link
Contributor Author

@mballance Thanks! Glad to contribute. I'm okay with my username / name (Alex Wilson) being listed in the ChangeLog file.

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.

None yet

2 participants