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)

11539_4.patch (3.6 KB) - added by eviatarbach 7 years ago.

Download all attachments as: .zip

Change History (23)

comment:1 Changed 9 years ago by kcrisman

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.

    def _equality_symbol(self):
        """
        Returns the equality symbol in Maxima.

        INPUT: none
        
        OUTPUT: string

        EXAMPLES::

             sage: maxima._equality_symbol()
             '='
             sage: var('x y')
             (x, y)
             sage: maxima(x == y)
             x=y
        """
        return '='

    def _inequality_symbol(self):
        """
        Returns the inequality symbol in Maxima.

        INPUT: none
        
        OUTPUT: string

        EXAMPLES::

             sage: maxima._inequality_symbol()
             '#'
             sage: maxima((x != 1))
             x#1
        """
        return '#'

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

    s = s.replace("#","!=") # a lot of this code should be refactored somewhere...

like we currently do.

comment:2 follow-up: Changed 9 years ago by 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.

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 mhansen

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 kcrisman

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 eviatarbach

  • 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 nbruin

The proposed lisp function makes me cringe. Wouldn't

(defprop mfactorial nil grind)

be a bit more elegant? That simply removes mfactorials 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: Changed 8 years ago by kcrisman

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 nbruin

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 kcrisman

#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 eviatarbach

  • 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 kcrisman

  • Authors set to Eviatar Bach, Nils Bruin
  • 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 eviatarbach

Ah yes, how silly of me. New patch fixes the issue and adds more tests.

comment:13 Changed 7 years ago by eviatarbach

  • Status changed from needs_work to needs_review

comment:14 Changed 7 years ago by kcrisman

  • 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).

Last edited 7 years ago by kcrisman (previous) (diff)

comment:15 follow-up: Changed 7 years ago by 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.

comment:16 in reply to: ↑ 15 Changed 7 years ago by nbruin

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:17 Changed 7 years ago by eviatarbach

  • Status changed from needs_work to needs_review

Thanks! New patch.

comment:18 Changed 7 years ago by nbruin

Fix looks good to me. We'll see if the patchbot is happy with it.

patchbot apply 11539_3.patch

Last edited 7 years ago by nbruin (previous) (diff)

comment:19 Changed 7 years ago by eviatarbach

Okay, fixed the two failing tests.

Changed 7 years ago by eviatarbach

comment:20 Changed 7 years ago by kcrisman

patchbot apply 11539_4.patch

comment:21 Changed 7 years ago by kcrisman

  • Status changed from needs_review to positive_review

Ran them by hand, all is well.

comment:22 Changed 7 years ago by jdemeyer

  • Merged in set to sage-5.10.beta5
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.