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

Fix incremental warnings with reluctant destabilization #709

Merged
merged 5 commits into from
May 2, 2022

Conversation

sim642
Copy link
Member

@sim642 sim642 commented Apr 27, 2022

Closes #708.

I tried the fix on the one test I added to confirm it works, but didn't experiment on more incremental tests with reluctant destabilization enabled.

Changes

  1. Fix old warnings not being removed by additionally deleting old nodes from superstable.
  2. Fix new warnings not being added for reluctantly unchanged functions by additionally querying those unknowns from postsolver.

@sim642
Copy link
Member Author

sim642 commented Apr 28, 2022

The now-failing incremental test added by @vesalvojdani actually leads to a bigger problem. There it appears that a warning is removed when it should not be, because the new version of the function also has the same problem. But we won't really be able to take the old warnings at the old nodes and somehow move them to new nodes, since we aren't aware of any correspondence (and there might even not be any).

So the real issue is that warnings from the new version of the function do not appear if reluctant destabilization detects that the return state doesn't change and avoids destabilizing it. I believe we would also have a problem if the old function had no warning, but the new version does, while the return state remains unchanged.

This is due to the unusual structure of reluctant destabilization solving: we solve some intermediate part of the constraint system and the destabilization doesn't lead all the way to the queried return node of the main function. Hence the latter remains superstable and interactive postsolving never does any work: the reluctantly solved function isn't re-evaluated for postsolving at all, so no warnings, no accesses and no verification of it.

@sim642 sim642 changed the title Fix warnings not removed with reluctant destabilization Fix incremental warnings with reluctant destabilization Apr 28, 2022
@sim642
Copy link
Member Author

sim642 commented Apr 28, 2022

I believe we would also have a problem if the old function had no warning, but the new version does, while the return state remains unchanged.

I added a third test also for this direction.

This is due to the unusual structure of reluctant destabilization solving: we solve some intermediate part of the constraint system and the destabilization doesn't lead all the way to the queried return node of the main function. Hence the latter remains superstable and interactive postsolving never does any work: the reluctantly solved function isn't re-evaluated for postsolving at all, so no warnings, no accesses and no verification of it.

To fix this I added a reluctant_vs list, where reluctantly unchanged unknowns are added. This list of unknowns is passed to the postsolver as additional queried variables, which forces the postsolver to directly iterate from those points. This should do all the postsolving tasks for the functions that were solved directly by reluctant destabilization.
On the last two tests it at least fixes the issue.

@sim642 sim642 mentioned this pull request May 2, 2022
5 tasks
@sim642
Copy link
Member Author

sim642 commented May 2, 2022

Interestingly, I already had part of this fix already in 7ad1f54 of #647 "just in case". This might've been why I was surprised by the issue: I had seen it under some conditions and fixed it, but it didn't yet lead to a fix like #713, so probably wasn't reviewed.

Copy link
Member

@vesalvojdani vesalvojdani left a comment

Choose a reason for hiding this comment

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

Works on the race-synthetic examples.

@sim642 sim642 merged commit db84ba3 into interactive May 2, 2022
@sim642 sim642 deleted the interactive-reluctant-warn branch May 2, 2022 15:53
@sim642 sim642 added this to the v2.0.0 milestone Aug 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Warnings not removed with reluctant destabilization and incremental postsolving
3 participants