-
-
Notifications
You must be signed in to change notification settings - Fork 205
Improvements to tackle wrong expression outputs #605
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
Conversation
|
You can get quite fancy with an Eventually, the limit will still kick in: We could issue a warning via |
mathics/core/expression.py
Outdated
| cost = sum(leaf.output_cost() for leaf in self.leaves) | ||
|
|
||
| if name == 'System`List': | ||
| return 2 + cost + len(self.leaves) # {a, b, c} |
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.
would it be better to compute the sum cost of leaves? Also, what about the size of ", " between leaves?
mathics/core/expression.py
Outdated
| elif name in _layout_boxes: | ||
| return cost | ||
| else: | ||
| return cost + 2 + self.head.output_cost() + len(self.leaves) # XYZ[a, b, c] |
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.
here also
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.
cost is the sum cost of leaves (I renamed it now). len(self.leaves) measures the size of ",", which could be one or two (it assumes for now). we could pass the separator size into make_boxes and then pass it on to output_cost, but it's bulky and then should we really measure it that way? If $OutputSizeLimit is understood as number of printable characters (without spaces and layout elements like fractions, parentheses, ...), then it makes sense to always measure 1 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.
In that case I guess technically it should be max(len(self.leaves) - 1, 0).
|
It seems like MMA kernels don't even observe |
|
So our approach is more efficient! Good to know |
mathics/core/expression.py
Outdated
| total_cost = 2 + cost_of_leaves + separator_cost * n_separators # {a, b, c}, [a, b, c] | ||
|
|
||
| if name != 'System`List': | ||
| total_cost += self.head.output_cost() + separator_cost * n_separators |
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.
doesn't this count the separators twice now?
|
This will need to be rebased on a revert of 8e14668. |
87662f3 to
52e46c7
Compare
ab12a77 to
6525df7
Compare
7a4d68a to
d41fae8
Compare
96ec58b to
e8a5440
Compare



see #602