Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
DOC-343: Unsat Cores Visualisation #153
DOC-343: Unsat Cores Visualisation #153
Changes from 12 commits
d298d6f
9c2d123
c5c2b25
ccc90fa
121c636
1c0c45a
7258670
3ff5fbb
d6bab66
bf981c9
10a5197
f634cf4
896f1e8
606d192
c91cdf1
81f22cc
6e6a77a
ec9d18f
70c1f12
254b5f0
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would add a glossary entry for unsat core in
/docs/user-guide/glossary.md
, which you can reference here with{term}`unsat core`
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As I think about this more, I think maybe we shouldn't use the words "unsat core", since the user isn't required to know what "unsat" means. Maybe we can just call this "coverage visualization" or something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I have completely avoided the concept of unsat cores.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Avoid using italic and bold inline markup.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please break long lines to 80 characters.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Users don't and shouldn't know what TAC is, and the term "verification condition" is probably pretty opaque too. I'm not even sure what to replace this with. Perhaps "we also visualize information about our internal representation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have completely removed the mentions about TAC and visualisation on TAC. Alex is now working on a dedicated TAC part of the documentation; I will add it there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like to include a contents block after the introduction of a multi-section page:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since I removed a lot of content (including sections), showing the contents does not feel right anymore
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would leave out most of this section.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
completely removed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be good to include this in the
Examples
repository and link to it so that people can run it themselves (and so we can rerun if the documentation needs to change).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a complex enough example that I'd consider turning on line numbers and referring to them in the text below (e.g. "we required
other != manager
on line 5").To do so, I think the syntax is
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would also link to the glossary (using
{term}`tautology`
) here. I'm pretty sure that there's already entries from the docs for the tautology checker. You could check the entries to see if it makes sense to link back here as well.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added a link to the glossary
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's nice to include short descriptions of the images in the braces, I guess for accessibility reasons.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would just move this to the alt text. If you really want it to be caption, here's how: https://myst-parser.readthedocs.io/en/latest/syntax/images_and_figures.html#figures-images-with-captions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't mention TAC - instead you can say that an individual line may perform multiple operations, only some of which are relevant.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rephrased
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think saying this two ways ("... that is ...") is confusing in this case. Instead, I might just say "the generated coverage information applies to the whole specification; if you want to generate coverage for a single rule or method, use
{ref}`--rule`
or{ref}`--method`
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And I might say this earlier, so that you can just include the impact of this in the bullets describing the colors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that the references for
--rule
and--method
are defined in #157There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No TAC
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would drop this section. End users should never ever have to look at a TAC dump
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alex gave webinar just a few weeks ago about using our TAC dumps to better understand and resolve timeouts. No one is saying that the end users have to look at a TAC dump; but if they can, it can help them a lot. Moreover, there is even a button (for long time already) to the TAC dump from the HTML report.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I figure we haven't synced on this enough...
My understanding, e.g. with Nurit and Shelly, lately was that we want to cautiously expose Security Engineers, and advanced end users to the graphical TAC dumps (not 100% sure about those groups, perhaps we can define it better).
Shelly has been making them nicer for that reason, and I have for example based some of the timeout feedback on them.
I think we're all in agreement that it would be nicer to lift everything user-facing to spec / source level, but this is, among other things, a developer-time tradeoff (not necessarily an aspect we advertise, but still..).
Another aspect is that TAC is closer to the VC, so for instance when it comes to understanding an SMT timeout, it might be helpful to see "what the solver is seeing" (e.g. when someone thinks they've summarized all nonlinear away, but haven't, they can see it in TAC).
The plan, afaiu, was to communicate a very rough working understanding about the TAC dumps.
As of now, this mostly includes looking at
We should probably have discussed / communicated this with you Mike in particular earlier -- I guess it didn't make the way across the ocean :-/ ...
(also not against a wider discussion, but I understand it's especially Mike's purview ..)
--> happy to discuss in any combination
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note also that I have an upcoming PR that adds a stub for that TAC dump documentation -- which will be followed up by a PR for that documentation itself ... (actually, I'll make a jira issue right now ..)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would drop most of the detail in this paragraph and just tell the user what they need to know: the basic coverage mode is approximate and may miscolor some statements, while the advanced coverage mode takes a long time to run.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This makes sense to me ... it also makes me wonder more generally (also wrt my upcoming DOC PRs):
Is there really no place in the docs for such elaborations :-)? (question to you, Mike)
-- maybe we could have a box (like the "note" and "todo" boxes) that indicates background information that isn't directly related to using the tool, but gives a deeper understanding of what's happening behind the scenes (?)
(sorry for hijacking this PR review a bit -- we can always break out into a zoom or something if this becomes longer)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed the information from here and added a small section
## Completeness
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should be in code font (which is not spell checked) instead of added to the word list.