Improve SeriesData format function #1627
Conversation
| SymbolSlot, | ||
| SymbolUndefined, | ||
| ) | ||
| from mathics.eval.makeboxes import format_element |
There was a problem hiding this comment.
I think format_element is no longer used.
| def pre_makeboxes(self, x, x0, data, nmin, nmax, den, form, evaluation: Evaluation): | ||
| if x0.is_zero: | ||
| variable = x | ||
| def format_series(self, x, x0, data, nmin, nmax, den, evaluation): |
There was a problem hiding this comment.
This is a good thing.
It brings up two issues.
First, I don't think we document anywhere in the Mathics3 developers' guide what format_xxx functions do, as we do with eval_xxx functions.
Second, notice that nowhere in here is self used. And that strongly suggests that this formatting-related code should not be here.
Similar to what we do with eval functions, this code simply call mathics.format.calculus. format_series, and the bulk of this code should be moved there.
Possibly the same is true with the other format_ functions, e.g. format_plus.
We can do this in another PR, but it would be better to do this sooner rather than later.
There was a problem hiding this comment.
There are more things to do with this module, starting from splitting it into smaller parts. Also, there are more things to do on the implementation of Series. This PR aims to simplify the formatting code slightly and make it compatible with what is to come.
Something that could be done in this PR is to move this code to a stand-alone function, to be called in the format_series method. But again, I am not sure where to put that code: should be in the mathics.eval module? or into the mathics.form module?
In any case, I am trying to avoid shaking the organization too much, to avoid merge conflicts with the code I am working on. We can review the code organization in a subsequent stage of the refactor.
There was a problem hiding this comment.
There are more things to do with this module, starting from splitting it into smaller parts. Also, there are more things to do on the implementation of
Series. This PR aims to simplify the formatting code slightly and make it compatible with what is to come. Something that could be done in this PR is to move this code to a stand-alone function, to be called in theformat_seriesmethod. But again, I am not sure where to put that code: should be in themathics.evalmodule? or into themathics.formmodule?
It goes in mathics.form. Things involving formatting, boxing, and rendering do not belong in eval, which is more intimately involved with numpy, mpmath, sympy, and lower-level Python libraries.
|
LGTM - see small lint change. It would be good to move out the That can be done in another PR, but it should be done sooner rather than later. |
| SymbolInfix, | ||
| ListExpression(regular, last), | ||
| String("+"), | ||
| Integer310, |
There was a problem hiding this comment.
This precedence value should be picked up from the precedence table.
There was a problem hiding this comment.
Let me change this when I move this code to mathics.form.formatvalues or something like that.
This PR reformulates the formatting of
SeriesDataexpressions, coming fromSeriesand related functions. Instead of working at the level of boxes, this implementation works at the level ofFormatValues, in a way that it can work with the new implementation ofOutputForm.