-
-
Notifications
You must be signed in to change notification settings - Fork 65
Fixing parsing for FormBox #545
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
base: master
Are you sure you want to change the base?
Changes from all commits
64c9295
3d83960
f15fe6c
73926ff
f3920f0
f6f3d1f
9a2680e
9485d9d
0cfd7d0
ca7dd9b
78fabca
65cfbac
8fcf8de
72bd2b3
89d74ee
f77fae3
dd57ebe
a67a6af
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -308,13 +308,53 @@ def p_RawLeftAssociation(self, token): | |
| def p_LeftRowBox(self, token): | ||
| self.consume() | ||
| children = [] | ||
| self.box_depth += 1 | ||
| self.bracket_depth += 1 | ||
| # If this does not happen, it would be because | ||
| # it was called when a `FormBox` (or any other) | ||
| # was found. More generally, we could use | ||
| # ``token.tag == "LeftRowBox" | ||
| # if there were other Tokens with a | ||
| # similar behaviour (see bellow). | ||
|
|
||
| if token.tag != "FormBox": | ||
| self.box_depth += 1 | ||
| self.bracket_depth += 1 | ||
|
|
||
| token = self.next() | ||
| while token.tag not in ("RightRowBox", "OtherscriptBox"): | ||
| while token.tag not in ("RightRowBox", "OtherscriptBox", "FormBox"): | ||
| newnode = self.parse_box(0) | ||
| children.append(newnode) | ||
| token = self.next() | ||
|
|
||
| # FormBox token has a particular behaviour: if it is found inside | ||
| # a RowBox, it splits the Rowbox in two pieces: the part at the | ||
| # left is taken as a "Format" and the part at the right is parsed | ||
| # as the Box to be formatted, in a way that | ||
| # \(a_1, a_2 ... \` b_1 b_2 ... \) is parsed as | ||
| # FormBox[RowBox[{b_1, b_2,...}], RowBox[a_1, a_2, ...]] | ||
| # This kind of parsing is not supported by the standard mechanism, | ||
| # so we need to processing it here instead of using a p_FormBox | ||
| # method. | ||
| # The strategy to deal with is that, when a "FormBox" tag is found, | ||
| # the collected elements are used to build the second argument of the | ||
| # output, and then the method is called again to parse the rest of the | ||
| # box as it had started at the "FormBox" tag. In this new call, | ||
| # neither `self.box_depth` or `self.bracket_depth` are going to be | ||
| # incremented, but are decremented at the end of the child expression. | ||
| if token.tag == "FormBox": | ||
| if len(children) == 0: | ||
| fmt = Symbol("StandardForm") | ||
| elif len(children) == 1: | ||
| fmt = children[0] | ||
| if type(fmt) is String: | ||
| fmt_name = fmt.value | ||
| if is_symbol_name(fmt_name): | ||
| fmt = Symbol(fmt_name) | ||
| else: | ||
| fmt = Node("Removed", String("$$Failure")) | ||
| else: | ||
| fmt = Node("RowBox", Node("List", *children)) | ||
| rest = self.p_LeftRowBox(token) | ||
| return Node("FormBox", rest, fmt) | ||
| if len(children) == 0: | ||
| result = String("") | ||
| elif len(children) == 1: | ||
|
|
@@ -840,20 +880,6 @@ def b_FractionBox(self, box1, token, p): | |
| box2 = self.parse_box(q + 1) | ||
| return Node("FractionBox", box1, box2) | ||
|
|
||
| def b_FormBox(self, box1, token, p): | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why was this removed?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Because with the current logic, to implement the behavior of
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, this is what I mean about the myopic kind of thing. FormBox needs to worry about precedence and the
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, I was aware about the possible issue with the precedence. So I checked it against WMA, and it also seems to neglect the operator precedence of What makes me think that this is the right approach (or at least a better approach than the current one) is that it does not break the existing tests, and it just makes a difference if a |
||
| q = misc_ops["FormBox"] | ||
| if q < p: | ||
| return None | ||
| if box1 is None: | ||
| box1 = Symbol("StandardForm") # RawForm | ||
| elif is_symbol_name(box1.value): | ||
| box1 = Symbol(box1.value, context=None) | ||
| else: | ||
| box1 = Node("Removed", String("$$Failure")) | ||
| self.consume() | ||
| box2 = self.parse_box(q) | ||
| return Node("FormBox", box2, box1) | ||
|
|
||
| def b_OverscriptBox(self, box1, token, p): | ||
| q = misc_ops["OverscriptBox"] | ||
| if q < p: | ||
|
|
||
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.
Every place there is some depth that increases by one, there should be another place that decreases the depth by one.
Where is that?
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.
If the tat is not
LeftRowBoxthen we never reach the decrement.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.
You are saying something more or less like what the code says. Why is the decrement not reached. What is the depth used for?
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.
The deep was used to handle nested expressions like
\( a b \( InputForm \` \(OutputForm \` c d\) e \) \)When a
\`is found, then the previous tokens are collected, and evaluated as aSymbol, a Box expression orStandardFormif there is no token before\`. The result of this step is stored in a variable, to be used as the second argument ofFormBox.Then, a new call to the method is done, with
FormBoxas a tag. In the new call, the next elements are processed until the bracket and box deeps reach 0. Then, these elements are collected into aStringor aRowBox. Then, the instance where\`was found takes the format and the other collected elements, builds aFormBox[], and returns it (before reaching the line where the decrements happen).I tried to find a way to put the increment and the decrement together, but it just was more involved than this approach: I should have allowed the increment of the deep variables, and then doing the decrement before returning the FormBox. I didn't think that it was clearer.
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 hope the new comment helps to understand the logic.
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.
Thanks for the description. Something about this seems a little to complicated and that there should be something that follows the specification in a little more direct way:
I would like to think about and reflect on this some more. I think we can make this cleaner, clearer and simpler.
What would be ideal is if we could do a cleanup step first - no changes to behavior yet.
Then add the list of tests that are wrong and that we want to fix.
And after this, then come up with a way to implement this.
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.
Actually, in WMA,
I am sure it is possible, just that after trying for a while, I didn't find a better way.
What would be a cleanup step? The list of (simple) tests to check are the tests that I added in test/core/parser/test_parser.py.
OK, in that case, I am going to put this as a draft for a while.