Skip to content
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

Improve frac parsing #23694

Merged
merged 2 commits into from Jul 1, 2022
Merged

Conversation

SethPoulsen
Copy link
Contributor

@SethPoulsen SethPoulsen commented Jun 27, 2022

References to other Issues or PRs

Brief description of what is fixed or changed

In the LaTeX parser, add support for bare numbers without brackets, like \frac12 for the fraction 1 / 2.
Also handles cases like turning \frac12y into 1 / 2 * y.

Other comments

Does not handle more complex cases where \frac is used without bracketing or with partial bracketing, such as
\frac1{2}, \frac{\sin{x}}2, etc. I may handle this in another PR if this one is well received.

Release Notes

  • parsing
    • LaTeX parser: support for fractions with no brackets.

@sympy-bot
Copy link

sympy-bot commented Jun 27, 2022

Hi, I am the SymPy bot (v167). I'm here to help you write a release notes entry. Please read the guide on how to write release notes.

Your release notes are in good order.

Here is what the release notes will look like:

This will be added to https://github.com/sympy/sympy/wiki/Release-Notes-for-1.11.

Click here to see the pull request description that was parsed.
<!-- Your title above should be a short description of what
was changed. Do not include the issue number in the title. -->

#### References to other Issues or PRs
<!-- If this pull request fixes an issue, write "Fixes #NNNN" in that exact
format, e.g. "Fixes #1234" (see
https://tinyurl.com/auto-closing for more information). Also, please
write a comment on that issue linking back to this pull request once it is
open. -->


#### Brief description of what is fixed or changed
In the LaTeX parser, add support for bare numbers without brackets, like `\frac12` for the fraction `1 / 2`. 
Also handles cases like turning `\frac12y` into `1 / 2 * y`. 

#### Other comments
~~Does not handle more complex cases where `\frac` is used without bracketing or with partial bracketing, such as
`\frac1{2}`, `\frac{\sin{x}}2`, etc. I may handle this in another PR if this one is well received.~~

#### Release Notes

<!-- Write the release notes for this release below between the BEGIN and END
statements. The basic format is a bulleted list with the name of the subpackage
and the release note for this PR. For example:

* solvers
  * Added a new solver for logarithmic equations.

* functions
  * Fixed a bug with log of integers.

or if no release note(s) should be included use:

NO ENTRY

See https://github.com/sympy/sympy/wiki/Writing-Release-Notes for more
information on how to write release notes. The bot will check your release
notes automatically to see if they are formatted correctly. -->

<!-- BEGIN RELEASE NOTES -->
* parsing
  * LaTeX parser: support for fractions with no brackets.
<!-- END RELEASE NOTES -->

Update

The release notes on the wiki have been updated.

frac:
CMD_FRAC L_BRACE upper = expr R_BRACE L_BRACE lower = expr R_BRACE;
frac: CMD_FRAC L_BRACE upper = expr R_BRACE L_BRACE lower = expr R_BRACE
| CMD_FRAC NUMBER ;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The purpose of this PR makes sense.
I'm not sure it's the best way to define syntax. CMD_FRAC NUMBER does more than a fraction should do, and that's why you have to deal with \frac1234 differently in convert_frac.
I think it should be something like \frac upper lower, where both upper and lower are either a single digit or a {expr}.
That way you immediately fix \frac1{2}.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the quick response!

I totally agree on principal, but is it even possible to do this in ANTLR?

The problem here is that the lexer and the parser really have to talk to each other, because 12 should almost always be lexed as a single NUMBER token, unless it immediately follows a \frac, in which case it would be better for it to be lexed as two separate DIGIT tokens. I guess you would need a predicate that can refer to the previous token.

Does ANTLR support this sort of thing? I am new to ANTLR myself.

edit: Thinking again, I'm not positive you need the lexer and parser to communicate, being able to save some state in the lexer might be sufficient. Still, I'm not sure if ANTLR supports this.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm giving a try myself and it seems harder than I supposed.
I'll update this PR if I succeed.
Actually I don't know much about ANTLR either, just try it intuitively using what I remember from the dragon book.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This stuff mentioned here about 'predicates' seems relevant: https://stackoverflow.com/questions/62469159/antlr-matlab-grammar-lexing-conflict

This seems to allow some sort of custom code execution during lexing, which is what we would need.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we make DIGIT terminal and NUMBER non-terminal, then we can use DIGIT in frac.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very clever! I love it.

@eagleoflqj
Copy link
Member

eagleoflqj commented Jun 28, 2022

frac's numerator or denominator accepts 1 digit only, and we have to use a terminal or non-terminal to represent it.
We don't want to use the old Number terminal or the new number non-terminal and split it. Nor do we want to use another non-terminal, because it definitely conflicts with number.
So I elevate Digit to terminal and use it.

@github-actions
Copy link

Benchmark results from GitHub Actions

Lower numbers are good, higher numbers are bad. A ratio less than 1
means a speed up and greater than 1 means a slowdown. Green lines
beginning with + are slowdowns (the PR is slower then master or
master is slower than the previous release). Red lines beginning
with - are speedups.

Significantly changed benchmark results (PR vs master)

Significantly changed benchmark results (master vs previous release)

       before           after         ratio
     [77f1d79c]       [f4691d30]
     <sympy-1.10.1^0>                 
+      96.5±0.4ms        175±0.6ms     1.81  sum.TimeSum.time_doit

Full benchmark results can be found as artifacts in GitHub Actions
(click on checks at the top of the PR).

@eagleoflqj
Copy link
Member

I'm going to merge if no objections.

@eagleoflqj eagleoflqj merged commit 7b38d1d into sympy:master Jul 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants