Opened 13 years ago
Closed 13 years ago
#6542 closed defect (fixed)
[with patch, positive review] tachyon ouput seems broken in sage-4.1
Reported by: | mhampton | Owned by: | mhampton |
---|---|---|---|
Priority: | blocker | Milestone: | sage-4.1.1 |
Component: | graphics | Keywords: | graphics, tachyon, raytracer |
Cc: | Karl-Dieter Crisman, wstein, Kelly Boothby, Minh Van Nguyen | Merged in: | Sage 4.1.1.rc0 |
Authors: | Marshall Hampton | Reviewers: | Tim Dumol |
Report Upstream: | N/A | Work issues: | |
Branch: | Commit: | ||
Dependencies: | Stopgaps: |
Description
As part of tracking this down, I am planning on adding doctests to more of the tachyon related files, which currently have low coverage.
Attachments (2)
Change History (14)
comment:1 Changed 13 years ago by
comment:2 Changed 13 years ago by
This might have been broken by 6269 or 6372. At any rate, the tostr function in tachyon.py is now missing a crucial piece. I am working on a patch.
Changed 13 years ago by
Attachment: | trac_6542_tachyon_tostr.patch added |
---|
fixes a mistaken deletion of critical functionality in tachyon's tostr function
comment:3 Changed 13 years ago by
Cc: | Karl-Dieter Crisman wstein Kelly Boothby added; Mike Hansen William Cauchois removed |
---|---|
Priority: | major → blocker |
Summary: | tachyon ouput seems broken in sage-4.1 → [with patch, needs review] tachyon ouput seems broken in sage-4.1 |
The patch fixes a mistaken deletion of critical functionality in tachyon's tostr function. Looks like a piece of this function was sliced off by mistake during the colorsys refactoring.
I will open a seperate ticket to improve the tachyon doctests; I think this patch should go in ASAP since tachyon is totally broken without it.
comment:4 Changed 13 years ago by
Cc: | Minh Van Nguyen added |
---|---|
Summary: | [with patch, needs review] tachyon ouput seems broken in sage-4.1 → [with patch, needs work] tachyon ouput seems broken in sage-4.1 |
This happened during the rebasing of #6372, moving the tachyon stuff. Perhaps this is a warning against restoring positive reviews immediately upon rebase, or perhaps not. In any case, the current patch needs a doctest to catch this - currently the function returns None every time this is called on a non-string, and that is probably why things didn't get caught before.
comment:5 follow-up: 6 Changed 13 years ago by
Well, yeah - the whole damn file needs doctests, which will take a while. But I really think this should be fixed as soon as possible.
I made ticket #6543 to fix the lack of doctests; that can build on this patch.
comment:6 Changed 13 years ago by
Replying to mhampton:
Well, yeah - the whole damn file needs doctests, which will take a while. But I really think this should be fixed as soon as possible.
True; I just meant to add a string with doctest that
tostr([5,4,3]) ' 5.0 4.0 3.0 '
as per Sage convention to check that it was fixed; that shouldn't be too bad. I'd do it myself but do not have a current Sage build available.
I made ticket #6543 to fix the lack of doctests; that can build on this patch.
Yes, I've been thinking about doing this for a while too.
comment:7 Changed 13 years ago by
Summary: | [with patch, needs work] tachyon ouput seems broken in sage-4.1 → [with patch, needs review] tachyon ouput seems broken in sage-4.1 |
---|
OK, I added a doctest to the tostr function.
comment:8 Changed 13 years ago by
Summary: | [with patch, needs review] tachyon ouput seems broken in sage-4.1 → [with patch, partial positive review, needs review] tachyon ouput seems broken in sage-4.1 |
---|
The function itself (obviously, since just replaces from before) works fine for me as is, and the documentation works fine in testing. Unfortunately, I can't currently merge it in a Sage build and do a test run, so needs further review to ensure that. This should be REALLY EASY to do for anyone who has a current Sage 4.1.
comment:9 Changed 13 years ago by
Owner: | changed from William Stein to mhampton |
---|
comment:10 Changed 13 years ago by
Reviewers: | → timdumol |
---|---|
Summary: | [with patch, partial positive review, needs review] tachyon ouput seems broken in sage-4.1 → [with patch, positive review] tachyon ouput seems broken in sage-4.1 |
Works perfectly for me -- Sage 4.1.
comment:11 Changed 13 years ago by
Typo in the word "seperated" on line 990:
-Converts vector information to a space-seperated string. +Converts vector information to a space-separated string.
comment:12 Changed 13 years ago by
Authors: | → Marshall Hampton |
---|---|
Merged in: | → Sage 4.1.1.rc0 |
Resolution: | → fixed |
Reviewers: | timdumol → Tim Dumol |
Status: | new → closed |
This problem was raised in this sage-support thread.