Opened 11 years ago
Closed 10 years ago
#12845 closed defect (fixed)
Incorrect doctest in sage/misc/functional.py
Reported by: | Michael Orlitzky | Owned by: | Burcin Erocal |
---|---|---|---|
Priority: | major | Milestone: | sage-5.1 |
Component: | symbolics | Keywords: | |
Cc: | Karl-Dieter Crisman | 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 )
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)
Change History (10)
Changed 11 years ago by
Attachment: | sage-trac_12845.patch added |
---|
comment:1 Changed 11 years ago by
Authors: | → Michael Orlitzky |
---|---|
Status: | new → needs_review |
comment:2 follow-up: 3 Changed 11 years ago by
Should there also be another ticket for fixing the bug of the real assumption?
comment:3 Changed 11 years ago by
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 11 years ago by
Cc: | Karl-Dieter Crisman added |
---|
comment:5 follow-up: 7 Changed 11 years ago by
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 11 years ago by
Attachment: | trac_12845-reviewer.patch added |
---|
comment:6 Changed 11 years ago by
Description: | modified (diff) |
---|---|
Reviewers: | → Karl-Dieter Crisman |
Status: | needs_review → positive_review |
Apply sage-trac_12845.patch and trac_12845-reviewer.patch.
comment:7 Changed 11 years ago by
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 10 years ago by
Merged in: | → sage-5.1.beta3 |
---|---|
Resolution: | → fixed |
Status: | positive_review → closed |
Add assumptions and use simplify() instead of simplify_full().