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 support for show statement in cat parser #742

Merged
merged 1 commit into from
Oct 10, 2024
Merged

Conversation

hernanponcedeleon
Copy link
Owner

No description provided.

@hernanponcedeleon
Copy link
Owner Author

These changes are need for eventual support of models like this one.

@ThomasHaas
Copy link
Collaborator

I wouldn't call this "support". The show (as) feature is essentially ignored and I think you should implement it like that (i.e., change nothing in the backend and just the parser skip it).
The reason is that you will need to implement it differently anyways, cause e.g. here shows three different variants, only one of which you "support":

show rf-reg // without renaming
show rf-mem as rf // with renaming
show co|fr as ca // can also show derived relations (even one that might not be relevant for the axioms)

@hernanponcedeleon
Copy link
Owner Author

The show in cat is used to enforce some edges to be displayed in the witness graphs. This will eventually be used in #732 thus I don't see why we should not already have this functionality implemented.

shows three different variants, only one of which you "support":

Now all 3 variants are supported.

Notice that for the last case, if we don't have a relation matching the term (as it is in the cat file you linked; co|fr neither exists in the included ctrl.cat), a new one will be created. Unless I am missing something, this will guarantee that a boolean variable for this term will exists in the solver if the eager encoding is used, and a graph with the definition will exists if the lazy method is used.

@ThomasHaas
Copy link
Collaborator

ThomasHaas commented Sep 27, 2024

The show in cat is used to enforce some edges to be displayed in the witness graphs. This will eventually be used in #732 thus I don't see why we should not already have this functionality implemented.

Ok, but then the implementation should be proper. I think adding show-as properties directly to Relation is not the way to go. To me, it seems the show-as feature is akin to metadata, i.e., somehow stored externally/orthogonally.
In this case, I would prefer to have something like a mapping Map<Relation, ShowAsInfo> in Wmm that manages this fact. ShowAsInfo is a record/class with an optional display name and maybe other goodies like display color, arrow type (e.g. dashed) etc. (only added if/when needed of course).

Btw. looking up the relation by name/term is pointless. The parser already has the relation in hand and could directly use it instead of taking its name/term, discarding the relation, and then forcing a lookup by term/name again (which currently also forces the unnamed relations to (wrongly) get their term as name!)

Notice that for the last case, if we don't have a relation matching the term (as it is in the cat file you linked; co|fr neither exists in the included ctrl.cat), a new one will be created. Unless I am missing something, this will guarantee that a boolean variable for this term will exists in the solver if the eager encoding is used, and a graph with the definition will exists if the lazy method is used.

No, it does not because we have dead relation elimination that kills unused relations. While it is unlikely that you want to display a relation that is unused in the axioms, I think you still should account for this fact.
And even if the relation is not killed, active set computation may avoid encoding parts of it (or all of it if unused in axioms!).
I also don't think you want to slow down solving (whether eager or lazy) just because you force encoding of otherwise unused show-as relations. I also think this is unnecessary as you could compute the show-as relations only once when printing the graph, assuming that at least the base relations are sufficiently encoded (this is likely the case, but not necessarily).
That means adapting our dead relation elimination pass (not too bad), possibly the active set computation (this could be wild), and maybe even CAAT so as to not compute over irrelevant graphs for consistency checking.

Signed-off-by: Hernan Ponce de Leon <[email protected]>
@hernanponcedeleon
Copy link
Owner Author

I decided to refer the decision of what to do with the information coming from show statements to #732 and keep this PR to a minimum by just allowing to parse cat files making use of show statements, but simply ignoring them

Copy link
Collaborator

@ThomasHaas ThomasHaas left a comment

Choose a reason for hiding this comment

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

Good idea :)

@hernanponcedeleon hernanponcedeleon merged commit 7ffb695 into development Oct 10, 2024
1 check passed
@hernanponcedeleon hernanponcedeleon deleted the showAs branch October 10, 2024 11:22
ThomasHaas pushed a commit that referenced this pull request Oct 10, 2024
Signed-off-by: Hernan Ponce de Leon <[email protected]>
Co-authored-by: Hernan Ponce de Leon <[email protected]>
@ThomasHaas ThomasHaas mentioned this pull request Nov 21, 2024
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