Visualize the actual proofs of the current problem with the TableauSVG component#79
Visualize the actual proofs of the current problem with the TableauSVG component#79jgonggrijp merged 31 commits intodevelopfrom
Conversation
I hate to disappoint, but I merely replaced export type ParseResponseData = {
ccg_parses: CCGParse[];
proofs: unknown[];
};If you need "any" help (see what I did there?) getting rid of the |
XanderVertegaal
left a comment
There was a problem hiding this comment.
Good work so far! I'm glad you added the comment about the proof being there (but hidden out of screen) for some of the longer problems. I was very confused. 😅 My solution for wide output was simply to add overflow: scroll to a couple of containers, so there is space to scroll horizontally. Still, I wonder if this is the neatest and most 'overzichtelijke' solution. Perhaps adding an external library with a zoom and drag interface would be a good addition at some point for both the tableaus and the (often very wide) syntactic parse result tables.
One note: I could not get this branch to work without commenting out this.parseResults = response.data.ccg_trees; in the AnnotationParseResults component. (This should be fixed once both of our branches are merged.)
As indicated in one of the comments. Let me know if you would like to delegate the type wrangling to me. I will continue my review after you have had time to add documentation to the tree2Tableau magic! 🧙♂️
| <button ngbNavLink> | ||
| <fa-icon [icon]="faTree" class="me-2" aria-hidden="true" /> | ||
| <span i18n>Tableau / Proof</span> | ||
| <span i18n>Proof / Entailment</span> |
There was a problem hiding this comment.
Much better! 'Tableau' is a representation but does not give information as to the actual content. (I don't call the syntactic parses "Table" either :P)
| <ng-template ngbNavContent> | ||
| <la-annotation-tableau /> | ||
| <section> | ||
| @if (proofs) { |
There was a problem hiding this comment.
Perhaps for later, but maybe you could add an @else block with something like "No entailment data available yet."
There was a problem hiding this comment.
Maybe we should factor that out from the AnnotationParseResults component in #78 and reuse it across tabs to make them look consistent.
| </li> | ||
| <li [ngbNavItem]="3"> | ||
| <button ngbNavLink> | ||
| <fa-icon [icon]="faTree" class="me-2" aria-hidden="true" /> |
There was a problem hiding this comment.
I tried looking for another icon we could use to differentiate the entailment tree from the contradiction tree, but no obvious candidates present themselves. Maybe link and linkSlash for entailment and contradiction, respectively?
(This is obviously not a high-priority item. 😅)
There was a problem hiding this comment.
There are symbols in Unicode that exactly convey the intended meaning: ⊤ and ⊥. I went with those.
| it('does not cause a stack overflow', () => { | ||
| let nltk: any = {node: 'Model'}; | ||
| const tableau: any = {nodes: []}; | ||
| for (let i = 0; i < 100000; ++i) { |
There was a problem hiding this comment.
TIL about post-increments and pre-increments! Neat!
| selector: "[tableau-tree]", | ||
| standalone: true, | ||
| imports: [TableauTerm], | ||
| imports: [TableauNode], |
There was a problem hiding this comment.
My IDE says that this import is unused. Is that true?
There was a problem hiding this comment.
The template of the tree component inserts instances of the node component. Maybe your IDE is failing to recognize that because the template is SVG instead of HTML?
| }; | ||
| } | ||
|
|
||
| const emptyTableau = (): any => ({nodes: []}); |
There was a problem hiding this comment.
Out of curiosity: any reason why you opt for an arrow function instead of a traditional one here? Brevity?
There was a problem hiding this comment.
Yes, if a function is just a single expression that fits on one line, I like to write it as an arrow.
| public proofs$ = this.parse$.pipe(map(extractProofs)); | ||
| } | ||
|
|
||
| function extractProofs(response: ParseResponse) { |
There was a problem hiding this comment.
Just a word of caution: catchError on line 21 returns null, so destructuring on line 32 will fail if that is the case. (This will undoubtedly be caught once we add proper typing.)
There was a problem hiding this comment.
Thanks for pointing that out to me! I just assumed that it would somehow magically prevent errors from being problematic in any way.
| return tree; | ||
| } | ||
|
|
||
| function nltkNode2tableauNode(nltk: any, tree: any) { |
There was a problem hiding this comment.
I await the documentation for this with bated breath! It feels very magical but at the same time there is not a lot of code, which is good.
Per review comment by @XanderVertegaal in #79.
|
@XanderVertegaal I responded to most of your comments so far and added comments for the Here is my full plan. Please let me know if you think I'm missing something or if you'd prefer me to proceed differently.
|
Per review comment by @XanderVertegaal in #79.
|
Your plan corresponds well to what we agreed, so please go ahead with the implementation. The only item on the list I would like you to clarify is 10: what do you mean with the 'no trigger unless viewing parse tab'? |
XanderVertegaal
left a comment
There was a problem hiding this comment.
Very neat, thank you for the documentation! Have a look at the alignment of the circles+symbols at the end of the trees. Otherwise, please carry on!
| @if(end) { | ||
| <svg:circle [attr.cx]="headX + 5" [attr.cy]="12" r="10" fill="salmon"/> | ||
| <svg:text #headText [attr.x]="headX" y="12" dominant-baseline="middle" font-weight="bold" fill="white">⛌</svg:text> | ||
| <svg:circle [attr.cx]="headX + 8" [attr.cy]="11" r="10" fill="salmon"/> |
There was a problem hiding this comment.
Argh! Which browser is this?
There was a problem hiding this comment.
By the way, I investigated using faLock and faLockOpen instead, but the icons would display with zero width and become invisible. I wasn't able to make it work and I suspect it is because the SVG icon is nested in an HTML element which is then again nested inside the larger SVG tableau.
There was a problem hiding this comment.
Argh! Which browser is this?
Never mind, it happens in my browser, too. Apparently I had to rebuild the container to see it.
There was a problem hiding this comment.
I see. For what it is worth: I am using Firefox.
| this.cdref.detectChanges(); | ||
| const width = this.treeDimensions.width; | ||
| if (width) this.element.nativeElement.parentElement.scroll({ | ||
| left: Math.max(0, width / 2 - 500) |
There was a problem hiding this comment.
Is 500 here chosen as an arbitrary (but sensible) minimal width?
There was a problem hiding this comment.
It's deliberately slightly more than half of the constrained body width. Without it, the center of the root would be on the left edge. I went with a slight bias towards the right because the description of the node starts on the left.
There was a problem hiding this comment.
You seem to have been right that the root is always at the middle of the top edge, by the way.
| // of the todo list. tsc is apparently unable to infer that `todo` is | ||
| // guaranteed to be nonempty on the next line, hence the silencing | ||
| // comment. | ||
| // @ts-ignore |
There was a problem hiding this comment.
I understand why the @ts-ignore is here, but instead of silencing it completely, maybe a type assertion todo.pop() as [any, any] or a non-null assertion todo.pop()! would be neater.
This is not something I insist on, but rather something you might want to keep in mind when you add the types.
| // guaranteed to be nonempty on the next line, hence the silencing | ||
| // comment. | ||
| // @ts-ignore | ||
| let task: [any, any] = todo.pop(); |
There was a problem hiding this comment.
Why are these lets rather than consts?
There was a problem hiding this comment.
Because I'm still used to var semantics and mentally hoisting them to the top of the function scope. 😅 (To be honest, I don't believe that let and const were an improvement to the language and I'm only using them in order to respect the local code convention.) I'll change it to const.
| // of | ||
| // https://en.wikipedia.org/wiki/Trampoline_(computing)#High-level_programming. | ||
| // The gist is this: instead of recursing, functions return pieces of work | ||
| // ("thunks") that should be executed next. The trampoline, which in this |
There was a problem hiding this comment.
Interesting. Would you say trampolining is generally superior to recursion when building nested datastructures like this? Are there any trade-offs, or areas in which recursion is preferable?
There was a problem hiding this comment.
When the datastructure might be arbitrarily deeply nested, you kind of "have" to use trampolining in order to avoid stack overflows. I became aware of this because of GHSA-qpx9-hpmf-5gmw and now cannot un-see that potential problem anymore, hence my vigilance. I'm actually planning to add a generalized _.trampoline to Underscore in order to provide a foundation for solutions like this one.
Suprisingly, there is no significant cost to trampolining versus recursion performancewise. The number of function calls is the same and the memory consumption is similar.
Conceptually, trampolining is one possible way to implement continuation passing style. Which has some theoretical elegance, but is more taxing on the brain than the conventional "direct" style. I would say that recursion is still preferable when you can get away with it, but in this case I went straight for trampolining because I think there might actually be a real risk that these trees get (slightly) too large.
| tree.nodes.push(node); | ||
| // Tease out the different parts of the stringified node payload. The | ||
| // following lines will be rewritten using regular expressions. | ||
| const lines = nltk.node.split('\n'); |
There was a problem hiding this comment.
Perhaps this string decoding could live in its own function (so we can test it in isolation), but you might want to wait for that until you have introduced the regexes.
Review suggestion in #79 Co-authored-by: Xander Vertegaal <73882506+XanderVertegaal@users.noreply.github.com>
The effect that I described to you previously, where the tableau would not render unless the "CCG / LLF" tab was selected while I pressed the "Parse and prove" button. However, this seems to have been solved already, possibly because I accepted your suggestion to add the I'll double-check after rebasing just to make sure, though. |
- Adopt same names for tree node parts as in LangPro API. - Distinguish node ID vs. numerical index of appearance. - Distinguish node vs. term vs. head. Mods and args contain terms, too.
Based on actual output from the LangPro container.
Per review comment by @XanderVertegaal in #79.
Per review comment by @XanderVertegaal in #79.
cf484da to
23b81c3
Compare
Co-authored-by: Xander Vertegaal <73882506+XanderVertegaal@users.noreply.github.com>
Per review comment by @XanderVertegaal in #79.
Review suggestion in #79 Co-authored-by: Xander Vertegaal <73882506+XanderVertegaal@users.noreply.github.com>
23b81c3 to
5899df2
Compare
I initially distinguished between terminal and internal trees as well, but applying this typing in the logic and convincing tsc that everything was fine became too unwildly.
|
@XanderVertegaal I force-pushed again because I needed to insert some of the new types early in history. Otherwise, all of the intermediate commits would be uncompilable. I did The only changes I still expect to make is type fixes in the templates and/or a simplification in the |
I initially distinguished between terminal and internal trees as well as between terminal nodes, internal nodes and unsupported nodes, but applying this typing in the logic and convincing tsc that everything was fine became too unwildly.
5899df2 to
a5d3aca
Compare
|
Done. |
| } | ||
| ] | ||
| } | ||
| public readonly tree = input.required<ProofTree>(); |
There was a problem hiding this comment.
(Utterly minor: I think inputs are conventionally put at the top of the component. At least, that is where most people will expect to find them.)
| } | ||
|
|
||
| set tree(value: any) { | ||
| set tree(value: ProofTree) { |
There was a problem hiding this comment.
It feels so good to see these anys replaced with actual types <3
Co-authored-by: Xander Vertegaal <73882506+XanderVertegaal@users.noreply.github.com>


Together with #78 and #77, I think this finally completes the work that started with #51.
The pre-existing "Tableau / Proof" tab is split into a "Proof / Entailment" and a "Proof / Contradiction" tab, which visualize the respective proofs for the current problem. Please read the commit messages for a quick summary of my approach.
The tests pass and it generally seems to work well. That being said, at the time of writing, there are still a few known problems with the code on this branch. More on this below.
For now, I suggest trying the branch as follows.
The default example from the old demo
A similar example that I just came up with
It works for large problems, too, but in most cases, the overflow on the tab content element is preventing the proof from being visible. For example, if you try the first SICK problem, the Entailment tab will look empty, but if you inspect the DOM, you will find that the tableau is actually there. There is still a visible visualization in the Contradiction tab, but it is truncated on the right. I have held off from fixing this because I already took a quick peek at #78 and noticed that @XanderVertegaal fixed an overflow problem on that brach, which is probably the same issue.
Other known issues that I think are also best solved in coordination with #78:
AnnotationMenuComponent, which passes the proof trees toTableauSVG, is not (yet) subscribed to theAnnotationService#proofs$observable. TheAnnotationsParseResultsComponentis subscribed to the underlying#parse$observable, however, and is therefore used (for now) to trigger the request to the backend. We could subscribe in two places, but it's probably more elegant (and perhaps also more efficient?) to coordinate a single place where this is done.anyall over the place. Without having seen the code, I suspect @XanderVertegaal already (partially) addressed this in Feature/langpro parse results #78.Other other known issues:
nltk2tableauconversion function deserves some solid documentation. I still want to add this before merging.TableauNodeComponentdoes not take the rule into account when calculating the width of leaf nodes (or of non-leaf nodes with very slim terms, for that matter). Also something I still want to fix before merging.Without the still to be added comments, I expect the conversion logic to be difficult to follow if not outright mysterious. That being said, you are welcome to review it anyway and let me know what you find most opaque/offensive. 😉