Opened 8 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 )
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 8 years ago by
comment:1 Changed 8 years ago by
- Status changed from new to needs_review
comment:2 follow-up: ↓ 3 Changed 8 years ago by
Should there also be another ticket for fixing the bug of the real assumption?
comment:3 in reply to: ↑ 2 Changed 8 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 7 years ago by
- Cc kcrisman added
comment:5 follow-up: ↓ 7 Changed 7 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 7 years ago by
comment:6 Changed 7 years ago by
- 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
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
- Merged in set to sage-5.1.beta3
- Resolution set to fixed
- Status changed from positive_review to closed
Add assumptions and use simplify() instead of simplify_full().