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:

Status badges

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)

trac_6542_tachyon_tostr.patch (581 bytes) - added by mhampton 13 years ago.
fixes a mistaken deletion of critical functionality in tachyon's tostr function
trac_6542_tachyon_tostr.2.patch (830 bytes) - added by mhampton 13 years ago.
new version with doctest

Download all attachments as: .zip

Change History (14)

comment:1 Changed 13 years ago by Minh Van Nguyen

This problem was raised in this sage-support thread.

comment:2 Changed 13 years ago by mhampton

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 mhampton

fixes a mistaken deletion of critical functionality in tachyon's tostr function

comment:3 Changed 13 years ago by mhampton

Cc: Karl-Dieter Crisman wstein Kelly Boothby added; Mike Hansen William Cauchois removed
Priority: majorblocker
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 Karl-Dieter Crisman

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 Changed 13 years ago by 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.

I made ticket #6543 to fix the lack of doctests; that can build on this patch.

comment:6 in reply to:  5 Changed 13 years ago by Karl-Dieter Crisman

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.

Changed 13 years ago by mhampton

new version with doctest

comment:7 Changed 13 years ago by mhampton

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 Karl-Dieter Crisman

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 mhampton

Owner: changed from William Stein to mhampton

comment:10 Changed 13 years ago by Tim Dumol

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 Minh Van Nguyen

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 Minh Van Nguyen

Authors: Marshall Hampton
Merged in: Sage 4.1.1.rc0
Resolution: fixed
Reviewers: timdumolTim Dumol
Status: newclosed
Note: See TracTickets for help on using tickets.