Fix evaluation of InterpretationBox with options #1607
Conversation
| def eval_create(self, reprs, expr, evaluation, options): | ||
| """InterpretationBox[reprs_, expr_, OptionsPattern[]]""" | ||
| if isinstance(reprs, EvalMixin): | ||
| reprs = reprs.evaluate(evaluation) |
There was a problem hiding this comment.
What are examples of this code getting used?
There was a problem hiding this comment.
the exampe is in test.format.format_test.m
There was a problem hiding this comment.
That's generic test code. Where in the YAML though?
There was a problem hiding this comment.
Yep, the YAML file contains the examples
There was a problem hiding this comment.
Yes, I figured that, but anything specific.
There was a problem hiding this comment.
When the evaluation is successful, the final expression shows the options in lexicographical order.
There was a problem hiding this comment.
Thanks for the information. This is sufficiently complicated and subtle that a comment should be added.
I've been trying to think of one, but I am not sure I fully understand.
Is the idea that reprs is either some sort of evaluatable expression (or is it just some of StyleBox)?
If it is in this former category, then we need to evaluate it to turn it into a box which will process and remove the options?
There was a problem hiding this comment.
This rule converts an Expression object into a InterpretationBox object. Expression does not have boxes_to_* methods, and InterpretationBox does not have an evaluate method. In order for the conversion happens, the rule must match with the expression. But the expression we have is of the form InterpretationBox[boxed, expr, opt1_, opt2_,...].
Also, InterpretationBox has a HoldAll attribute, so if the first element is not a BoxElementMixin, we need to evaluate it first.
There was a problem hiding this comment.
Ok. Let's add this as a comment. I can do that tomorrow, along with the other small doc changes.
There is a lot of code in this PR that feels mysterious and bottom-up, as opposed to there beings some higher-level principle that can be used to derive the details coded.
Maybe later we will have a simpler model for expressing InputForm and how that relates IntrpretationBox. IF that happens, it might be that it fits so well that we don't need to make a test like this, or the test feels more obvious.
There was a problem hiding this comment.
This mechanism of conversions has been around for a while, but probably requires better documentation. The main idea is that MakeBoxes[] does not produce Expression objects, but BoxElementMixin objects. *Box expressions should then be evaluated to these kind of objects. The problem that this PR tries to fix is that this conversion fails for InterpretationBox expressions having optional parameters.
mathics/builtin/box/layout.py
Outdated
|
|
||
| def eval_create(self, reprs, expr, evaluation, options): | ||
| """InterpretationBox[reprs_, expr_, OptionsPattern[]]""" | ||
| # If the first element is not a litera, this |
There was a problem hiding this comment.
litera -> literal
(I am not seeing the +/- commit button, or I'd suggest a commit.)
Thanks for the additional comments and clarification. This helps a lot.
Co-authored-by: Juan Mauricio Matera <[email protected]>
…_boxlayout_tweaks
|
LGTM. Many thanks for sticking with this. |
Another round of fixes. These are necessary for
format_test.mwork withInputForm