Repository navigation
Read factorial(x) as the factorial rather than as a product (#733) - #737
Merged
Merged
Conversation
`factorial` is the name the function has in sympy, MATLAB, Mathematica and Python's math module, and the grammar did not have it -- while having the function already, as the postfix `x!` and as `MathS.Factorial`. Nothing was missing but the spelling. A one-argument call under a name the grammar does not know never errors: it falls through to the implicit multiplication that lets `a(b + c)` mean `a * (b + c)`. So `factorial(5)` came out as the product `factorial * 5`, an undeclared variable times 5, silently, where the answer is 120. One rule, mapping to `MathS.Factorial(arg)`, placed next to `gamma(` -- the same fact one shift along, which the grammar already had -- so that `factorial(x)` and `x!` parse to one tree rather than each being separately defined. Only the exact name followed by a bracket is the function, as for every other function in the grammar. On its own `factorial` is still an ordinary variable, `factorial2` is still the implicit power that `x2` means, and `factoriall(x)` and `factorial3(x)` are still the implicit products they were. Tests pin all of them, and the postfix `x!` is untouched. This is the last of the defect half of #733, found by sweeping the rest of `MathS`'s public surface past the parser rather than the CAS spellings. The sweep is otherwise clean: every hyperbolic spelling, `signum`, `abs` and `gamma` are read correctly, and `taylor`, `sinc` and `conjugate` name nothing the library has as a node. #733 stays open for the names it does not have -- `floor`, `ceil`, `round`, `min`, `max`, `gcd`, `lcm` -- since each wants its own judgement about whether the library should have the function at all. Measured: `factorial(5)` from `factorial * 5` to 120, `factorial(0)` 1, `factorial(1)` 1, `factorial(6)` 720, and `factorial(x)` now the same tree as `x!`. Parser regenerated with antlr-4.13.1 and the post-processor. Full suite 4905 passed / 0 failed with this change alone -- the 16 above master's 4889 are the ones added here -- F# 130/130, corpus 112/117 with 0 wrong, 0 error, 0 timeout. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Part of #733 -- the last of its defect half.
factorialis the name the function has in sympy, MATLAB, Mathematica and Python'smathmodule, and the grammar did not have it -- while having the function already, as the postfixx!and asMathS.Factorial. Nothing was missing but the spelling.A one-argument call under a name the grammar does not know never errors: it falls through to the implicit multiplication that lets
a(b + c)meana * (b + c). Socame back as a product, silently, where the answer is
120.Fix
One rule, mapping to
MathS.Factorial(arg), placed next togamma(-- the same fact one shift along, which the grammar already had -- so thatfactorial(x)andx!parse to one tree rather than each being separately defined.Only the exact name followed by a bracket is the function, as for every other function in the grammar:
factorial(x)x!factorialfactorialfactorial2factorial ^ 2x2means, as beforefactoriall(x)factoriall * xa(b + c)means, as beforefactorial3(x)factorial ^ 3 * xTests pin all of them, and the postfix
x!is untouched.How it was found
By sweeping the rest of
MathS's public surface past the parser, rather than the CAS spellings that #733 came from. That sweep is otherwise clean: every hyperbolic spelling,signum,absandgammaare read correctly, andtaylor,sincandconjugatename nothing the library has as a node.#733 stays open for the names the library genuinely does not have --
floor,ceil,round,min,max,gcd,lcm-- since each wants its own judgement about whether the library should have the function at all.Measured
factorial(5)fromfactorial * 5to120;factorial(0)1;factorial(1)1;factorial(6)720;factorial(x)now the same tree asx!;factorial(5)agrees withgamma(6).Parser regenerated with antlr-4.13.1 and the post-processor, as in #734. Full suite 4920 passed / 0 failed, F# 130/130, corpus 112/117 with 0 wrong, 0 error, 0 timeout.
🤖 Generated with Claude Code