Opened 9 years ago
Closed 7 years ago
#11539 closed defect (fixed)
Sage incorrectly interprets factorials in equations.
Reported by: | mhansen | Owned by: | burcin |
---|---|---|---|
Priority: | critical | Milestone: | sage-5.10 |
Component: | symbolics | Keywords: | |
Cc: | dsm | Merged in: | sage-5.10.beta5 |
Authors: | Eviatar Bach, Nils Bruin | Reviewers: | Karl-Dieter Crisman |
Report Upstream: | N/A | Work issues: | |
Branch: | Commit: | ||
Dependencies: | Stopgaps: |
Description
sage: maxima(factorial(x) == 0).sage() x != 0
Running the following two commands to change how factorials are printed in Maxima will fix this:
sage.calculus.calculus.maxima.eval(r":lisp (defun msize-factorial (x l r) (setq l (msize (cadr x) (revappend '(#\f #\a #\c #\t #\o #\r #\i #\a #\l #\() l) nil 'mparen 'mparen) r (list 1 #\) ) ) (list (+ (car l) (car r)) l r)) ") sage.calculus.calculus.maxima.eval(r":lisp (defprop mfactorial msize-factorial grind)")
We should run these when the Maxima interface is started.
Attachments (1)
Change History (23)
comment:1 Changed 9 years ago by
comment:2 follow-up: ↓ 4 Changed 9 years ago by
If you wanted to fix this on the Sage side of things, you'd need to make sage.misc.parser.Parser accept "#" as an inequality symbol. This is because "x!=0" is ambiguous.
I posted the fix on the Maxima side since that was a change that the user could make to a running Sage and not have to patch it, etc.
comment:3 Changed 9 years ago by
This works by just changing the function that Maxima calls when it goes to print a factorial function.
comment:4 in reply to: ↑ 2 Changed 9 years ago by
Replying to mhansen:
If you wanted to fix this on the Sage side of things, you'd need to make sage.misc.parser.Parser accept "#" as an inequality symbol. This is because "x!=0" is ambiguous.
Right. But I was anticipating having the Sage translator in calculus.py turn "!" into " ! " or something like that.
sage: x ! = 0 ------------------------------------------------------------ File "<ipython console>", line 1 x ! = Integer(0) ^ SyntaxError: invalid syntax
since in Maxima the exclamation point only means this. Then one could somehow smartly turn "foo !" into "factorial(foo)", though as I say above it might be tricky determining what foo is.
I posted the fix on the Maxima side since that was a change that the user could make to a running Sage and not have to patch it, etc.
Yes, for ask.sagemath.org that is a good fix. I'm just not sure I want to make such a change in Maxima - mightn't this change some internal behavior there? Plus, we try not to fix upstream if it isn't really a bug. More philosophical than practical, I agree :)
comment:5 Changed 8 years ago by
- Priority changed from major to critical
Here's another problem with factorials and Maxima:
sage: var('y') sage: (factorial(x)==y).solve(x) TypeError: unable to make sense of Maxima expression '[x!==y]' in Sage
Changed this to critical, since the first example can actually produce wrong answers.
comment:6 Changed 8 years ago by
The proposed lisp function makes me cringe. Wouldn't
(defprop mfactorial nil grind)
be a bit more elegant? That simply removes mfactorial
s special "grind" (maxima print) property, so that the default method of printing takes effect again.
This should be a quite safe change to make to maxima. We're doing much worse patching in the maxima_lib interface. Since "!" would be the only postfix operator, it's a bit of wasted effort to extend the parser to accept it.
comment:7 follow-up: ↓ 8 Changed 8 years ago by
I like the idea! But this isn't working for me - am I making some newbie mistake?
(%i1) :lisp (+ 2 2) 4 (%i1) :lisp (defprop mfactorial nil grind) NIL (%i1) factorial(x); (%o1) x!
And
sage: sage.calculus.calculus.maxima.eval(r":lisp (+ 2 2)") RuntimeError: ECL says: THROW: The catch MACSYMA-QUIT is undefined.
though perhaps putting it in the initialization would not cause this...
comment:8 in reply to: ↑ 7 Changed 8 years ago by
Replying to kcrisman:
I like the idea! But this isn't working for me - am I making some newbie mistake?
Indeed. This only affects display2d:false
printing. The nicer print modes (including latex) will print as usual:
(%i1) factorial(x); (%o1) x! (%i2) :lisp (defprop mfactorial nil grind) NIL (%i2) factorial(x); (%o2) x! (%i3) display2d:false; (%o3) false (%i4) factorial(x); (%o4) factorial(x)
And it doesn't work
sage: sage.calculus.calculus.maxima.eval(r":lisp (+ 2 2)") RuntimeError: ECL says: THROW: The catch MACSYMA-QUIT is undefined.
Yes, maxima_lib doesn't perfectly mimmick the expect interface that way. Apparently we're not hooking into a part of the parser that accepts directives. However,
sage: sage.calculus.calculus.maxima.lisp("(remprop 'mfactorial 'grind)")
and
sage: M=Maxima() sage: M.lisp("(remprop 'mfactorial 'grind)") '"";\r\n\r\nT\r\n<sage-display>('
both work, so there's a workaround.
Note that we can use remprop
to actually remove a property. In CL you have to work quite hard to see the difference between a NIL-valued property and a non-existent property. Most maxima symbols do not have a GRIND
property, though, so perhaps removing it is the more proper thing to do. The defprop
must be a Maxima macro, by the way. It's not standard CL. But then ... many parts of Maxima predate CL.
comment:9 Changed 7 years ago by
#14352 was a duplicate. For reference:
sage: factorial(x) == 6 factorial(x) == 6 sage: _.simplify() x != 6 bool((factorial(x) == 6).simplify().subs(x=2))
Since that put this on the radar again... so, presumably all one would have to do is run
(remprop 'mfactorial 'grind')
at the same place we do other initialization, maybe after
ecl_eval('(defun principal nil (cond ($noprincipal (diverg)) ((not pcprntd) (merror "Divergent Integral"))))')
in sage/interfaces/maxima_lib.py, and put a couple doctests in to verify these things work? Actually, looks like we'd need something in maxima_abstract.py or maxima.py as well to fix the very first example.
comment:10 Changed 7 years ago by
- Status changed from new to needs_review
It seems to be fixed just by adding the line in maxima_lib.py
.
comment:11 Changed 7 years ago by
- Reviewers set to Karl-Dieter Crisman
- Status changed from needs_review to needs_work
I like this, but I still get
sage: sage: maxima(factorial(x) == 0).sage() x != 0
because of course that is not using the library interface, and really both of them need to work, as I say in comment:9.
Actually, looks like we'd need something in maxima_abstract.py or maxima.py as well to fix the very first example.
You might as well also add your first example, Eviatar:
sage: var('y') sage: (factorial(x)==y).solve(x) [factorial(x) == y]
But nice work on the whole just getting this in.
comment:12 Changed 7 years ago by
Ah yes, how silly of me. New patch fixes the issue and adds more tests.
comment:13 Changed 7 years ago by
- Status changed from needs_work to needs_review
comment:14 Changed 7 years ago by
- Cc dsm added
- Status changed from needs_review to needs_work
I like this. Tried a few other things but it seems to work fine. However, now I get
sage -t sage/calculus/calculus.py ********************************************************************** File "sage/calculus/calculus.py", line 1743, in sage.calculus.calculus.symbolic_expression_from_maxima_string Failed example: sefms("x != 3") == SR(x != 3) Expected: True Got: False ********************************************************************** 1 item had failures: 1 of 17 in sage.calculus.calculus.symbolic_expression_from_maxima_string [375 tests, 1 failure, 6.10 s] ---------------------------------------------------------------------- sage -t sage/calculus/calculus.py # 1 doctest failed
That's because of the change
sage: from sage.calculus.calculus import symbolic_expression_from_maxima_string as sefms sage: sefms("x!=3") x != 3
to
sage: sefms("x!=3") factorial(x) == 3
I'm not sure whether we should just remove this doctest or whether there is an appropriate "fix". I've cc:ed Doug, who worked on #8969 and might have a comment about this (though I guess I was the one who originally proposed this test).
comment:15 follow-up: ↓ 16 Changed 7 years ago by
There's been no response for a while; do you think I could just remove the test? It would be good to get this patch into the next release.
comment:16 in reply to: ↑ 15 Changed 7 years ago by
Replying to eviatarbach:
There's been no response for a while; do you think I could just remove the test? It would be good to get this patch into the next release.
No, change the test to
sage: sefms("x!=3") == (factorial(x) == 3) True
It's good edge behaviour to test on: Someone used to python would read this string one way, but to maxima it means something else. We're now actually getting the meaning that maxima attaches to this string. The result we were getting before was rather Babylonian.
comment:18 Changed 7 years ago by
Fix looks good to me. We'll see if the patchbot is happy with it.
patchbot apply 11539_3.patch
comment:19 Changed 7 years ago by
Okay, fixed the two failing tests.
Changed 7 years ago by
comment:20 Changed 7 years ago by
patchbot apply 11539_4.patch
comment:21 Changed 7 years ago by
- Status changed from needs_review to positive_review
Ran them by hand, all is well.
comment:22 Changed 7 years ago by
- Merged in set to sage-5.10.beta5
- Resolution set to fixed
- Status changed from positive_review to closed
Hmm, not sure I agree with this solution. Also, a reference to why this works would be nice :) I am impressed!
But wouldn't it be better to figure out a way to improve our interface rather than mess with Maxima? After all, '!' only occurs in Maxima for factorial, so it should be possible to parse this correctly. We'd have to grab whatever expression came before it, either in parentheses (which could have several levels) or a numeric type. Hmm.
Also, this is another good excuse to think about moving the Maxima parsing stuff out of the calculus file into one of the Maxima files where it belongs... like, it would make more sense to use the above code than do
like we currently do.