-
-
Notifications
You must be signed in to change notification settings - Fork 59
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
2d print #1155
Draft
mmatera
wants to merge
19
commits into
master
Choose a base branch
from
2d_print
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Draft
2d print #1155
Changes from 1 commit
Commits
Show all changes
19 commits
Select commit
Hold shift + click to select a range
8e42c96
not ready
mmatera d990b15
second round
mmatera 2b51c37
tmp
mmatera 9595b07
another little step
mmatera 161705b
merge
mmatera 14889d7
partial
mmatera 331c9a7
* Improving precedence comparison.
mmatera 03dc038
partial
mmatera c37afe7
rocky's observations
mmatera bee14ae
2d_print support
mmatera 845fb99
working
mmatera 760e04e
mypy
mmatera 19c9904
Merge branch 'master' into 2d_print
mmatera a591742
removing testing function
mmatera c5faf8f
Merge branch 'master' into 2d_print
mmatera 1cd0bc8
moving and renaming modules
mmatera baa276e
fixing formatting in fractions and square roots
mmatera f91168f
Merge branch 'master' into 2d_print
mmatera af8e4c2
Merge branch 'master' into 2d_print
mmatera File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Instead of adding more original code, please look into hooking into SymPy's character-based formatting mechanism.
Otherwise, this may be another kind of thing where effort is put into creating something that is later removed, because there is something that is more likely to be more complete and that we won't have to maintain.
If it turns out that we can't use SymPy's character-based printing, then we can go down this road.
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.
Indeed, the plan is to replace the use of
mathics.format.pane_text
bysympy.printing.pretty.stringpict
, which is something that I already started to do.What is not possible to do is just using
sympy.pretty(expression.to_sympy())
because the translation function which is useful for evaluation, is not useful for formatting.For example
produces
while
is not even able to identify the integrate symbol:
In any case, the purpose of this PR is to
prettyForm
object.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.
This is clearly wrong because
expr
needs to be boxed first. Calling SymPy formatting routines are triggered by the formatting process of boxed expressions.This is setting up a strawman or superficial argument only to be able to shoot it down.
The time spent adjusting the bar in a division I think is better spent towards getting to the skeleton of a possible solution. The plan that you wanted written down has you working on revising Boxing. When that is in place, we might be in an even better position to work on the boxing to formatting step needed in character-based printing.
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.
Please, do not take this in a wrong way: the example was just a way to show some of the challenges in hooking up
sympy.pretty
: even at the level of symbols, a preprocessing must be done. Probably what is more easy to hook is thesympy.printing.pretty.stringpict.prettyForm
, which is quite analogous to what I put inpane_text
.sympy.pretty
works using asympy.printing.pretty.PrettyPrinter
object that does something similar to what I did inmathics.format.prettyprint
.OK, but the skeleton of the solution that I propose is already here: When an expression is wrapped by
OutputForm
(even if$Use2DOutputForm
is set toFalse
) the formatting process follows a sequence closer to the one (I think) WMA follows.In any case, I wanted to put this here, because I will need it to present my case when I propose the other changes.
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 was aware of the challenges and the process well before you started this PR. "preprocessing" is the wrong word/concept. In the formatting process, objects like SymPy symbols get transformed to strings.
Except more effort, time, and thought was probably put into the
sympy.printing.pretty.PrettyPrinter
object. It may be that it is more elucidating for you to write some code so you understand the basic concepts rather than look at someone else's code. For me though this kind of thing is more of a distraction and possibly a dangerous activity, because I get the feeling that if I don't mention something, this kind of thing will get into the code base and then we'll want to remove it later on. We have seen this kind of thing too often. Furthermore, we haven't dug out of the previous messes fully yet.There are probably very many situations that aren't covered and haven't been considered. And in the first few things I tried, I saw differences.
BTW, I don't like the term "2D". This is character-based output. Most output, such as SVG, MathML, and LaTeX, is 2D.
Personally, I would prefer if you discuss what changes you want to propose at a high level first. If I need detailed code to understand, then we can code this out. If you need to write some sample code for yourself , sure do that. But unless others express interest in seeing this, these branches don't help me in a positive way. Rather, it feels negative because I see flailing about where it feels to me there shouldn't be flailing like this.
In my view, the developer docs describe in pretty good detail how the system transforms M-expressions to boxed-expressions, and then to formatted output.
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.
OK, but for naming functions, it would be a little bit long, isn't it? Here I didn´t want to call it "OutputForm", because in all the other places, "OutputForm" is still a "one-dimensional using only keyboard characters" which most of the time is the same than
InputForm
, but with spaces between infix operators and operands.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 any case, the question is: supposing we found and agree on an implementation of this "two-dimensional using only keyboard characters", is it something that we want to have (at least optional) available in the command line interface?
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 guess what I was trying to say is that if you have to drop something in the description, dropping "character-based" is bad, in the same way that condensing "strawberry" to "straw" rather than "berry" is not helpful.
"Character2D" is not too long. But the word "character" is as important as 2D.
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.
It feels to me that we are thinking about this the wrong way. To me, this is like asking if TeXForm should be in the command-line interface. And whether we should be able to return MathML formatted output in the command-line interface.
To me, the focus of the implementation should be on how things are boxed and formatted in a generic and general way. Not about what is appropriate for a particular front end. The current implementation is lacking in the generic and general nature. It was particularly more evident when this was in
mathics.core
which 2D character-based formatting is totally inappropriate for.Here is another example from our code base. We have these whacky and complicated regular expressions for handling doctests inside of docstrings. I imagine the person that started this may have thought it cool to recreate sphinx using regular expressions. Those regular expressions use some pretty advanced and little-used tagging mechanisms. If you want to show off how clever you can code, great. But as far as handling the underlying problem in a uniform, maintainable, and comprehensible way, this code totally fails.
Let's defer character-based 2D output formatting until after we have Boxing under control. Formatting is intimately tied to Boxing. And if we have a good Boxing mechanism, I think you'll see how easily character-based 2D formatting falls out from 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.
TeXForm is useful and needed at last to generate the PDF documentation. MathML is used in the Django front-end. PrettyPrint is something "pretty" but maybe is a waste of time to implement/maintain it. For example, probably we do not want to use to check doctests, because writing the expected results would be awkward.
Yes, but at least for me, it helps me to think why WMA implements boxing as it does. And this is all the reason I wrote and put this here.
Now I am putting another PR, where all the Character2D code is stripped away.
OK
Sure. In any case, if you are OK with it, I will leave this here for a while.