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

Wrong trace delay - fix 2035819 #45

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

mtygesen
Copy link
Contributor

@mtygesen mtygesen marked this pull request as ready for review June 14, 2024 08:16
@srba
Copy link
Member

srba commented Jun 16, 2024

One could have set the max constant to 0 already at line 122 with the same effect (at least from a quick look). I am though afraid that this could kill some of the optimizations as if the constant is -1 the engine regards it essentially as untimed place. I think that maybe one could set it to 0 at line 122 only if we ask for the fastest trace, otherwise we could leave it at -1. What do you think @petergjoel ?

@srba srba requested a review from petergjoel June 16, 2024 19:11
@srba srba self-requested a review June 17, 2024 19:26
@petergjoel
Copy link
Member

Unfortunately I think this fix is coincidental (or at least not the right fix). AFAIK, the fastest trace is preserved even by the local pruning, so changing the max-constant should not impact the existence of the fastest trace.

The adverse effect of this fix is an exponential blowup in the statespace when we have untimed places (they can now have the value 0 and 1 as delay), so it is not a light cost, even if the fix is correct.

But it indicates that something fishy is going on during the extrapolation/pruning operation.
I still have a hunch that it is down to the next-state computation order somehow messing up the search order OR the stored trace.

Some things to check:

  1. on termination, is the search stack still neatly ordered?
  2. do we add a "predecessor" in the trace too early due to ordering? (check https://github.com/TAPAAL/verifydtapn/blob/main/src/DiscreteVerification/DataStructures/PWList.cpp line PWListHybrid::add Specifically, should we sometimes store another predecessor as the parent, not just the first?

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