Opened 7 years ago

Closed 7 years ago

#12845 closed defect (fixed)

Incorrect doctest in sage/misc/functional.py

Reported by: mjo Owned by: burcin
Priority: major Milestone: sage-5.1
Component: symbolics Keywords:
Cc: kcrisman Merged in: sage-5.1.beta3
Authors: Michael Orlitzky Reviewers: Karl-Dieter Crisman
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description (last modified by kcrisman)

We currently have,

sage: a, b, c = var("a, b, c")
sage: v = vector([a, b, c])
sage: bool(norm(v).simplify_full() == sqrt(a^2 + b^2 + c^2))
True

which is true enough if a,b,c are real. The current doctest "works" because somewhere along the line, simplify_full() assumes that they're real. So in effect, this doctest is confirming incorrect behavior.

The proper fix is to explicitly assume that a,b,c are real.


Apply sage-trac_12845.patch and trac_12845-reviewer.patch.

Attachments (2)

sage-trac_12845.patch (941 bytes) - added by mjo 7 years ago.
Add assumptions and use simplify() instead of simplify_full().
trac_12845-reviewer.patch (539 bytes) - added by kcrisman 7 years ago.

Download all attachments as: .zip

Change History (10)

Changed 7 years ago by mjo

Add assumptions and use simplify() instead of simplify_full().

comment:1 Changed 7 years ago by mjo

  • Authors set to Michael Orlitzky
  • Status changed from new to needs_review

comment:2 follow-up: Changed 7 years ago by novoselt

Should there also be another ticket for fixing the bug of the real assumption?

comment:3 in reply to: ↑ 2 Changed 7 years ago by mjo

Replying to novoselt:

Should there also be another ticket for fixing the bug of the real assumption?

Yes, there are at least two, #12737 and #12780. I separated this patch out because I wound up fixing the same doctest twice.

The first bug, "fixed" by #12737 is that simplify_radical() ignores the domain anyway and goes nuts. "Simplify" there is really a misnomer, so the fix that I settled on was to remove it from simplify_full():

sage: bool(norm(v).simplify_radical() == sqrt(a^2 + b^2 + c^2))
True

The second more sneaky bug, #12780, is that simplify_log() sets the Maxima domain to real before it performs its simplifications. Thus,

sage: bool(norm(v).simplify_log() == sqrt(a^2 + b^2 + c^2))    
True

I think the fix there is straightforward: don't do that. This was the only doctest affected by not setting the domain to real during simplify_log().

comment:4 Changed 7 years ago by kcrisman

  • Cc kcrisman added

comment:5 follow-up: Changed 7 years ago by kcrisman

Do you have to forget() anything for doctests to work properly? I think it's good form to do so at the end of any assumptions tests...

Changed 7 years ago by kcrisman

comment:6 Changed 7 years ago by kcrisman

  • Description modified (diff)
  • Reviewers set to Karl-Dieter Crisman
  • Status changed from needs_review to positive_review

Apply sage-trac_12845.patch and trac_12845-reviewer.patch.

comment:7 in reply to: ↑ 5 Changed 7 years ago by mjo

Replying to kcrisman:

Do you have to forget() anything for doctests to work properly? I think it's good form to do so at the end of any assumptions tests...

Yeah, thanks, I just forgot to add it.

comment:8 Changed 7 years ago by jdemeyer

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