Opened 5 years ago
Closed 5 years ago
#16226 closed defect (fixed)
Tachyon plot produces invalid file
Reported by:  niles  Owned by:  

Priority:  major  Milestone:  sage6.3 
Component:  graphics  Keywords:  tachyon, plot3d 
Cc:  kcrisman  Merged in:  
Authors:  Niles Johnson  Reviewers:  Frédéric Chapoton 
Report Upstream:  N/A  Work issues:  
Branch:  543c6ef (Commits)  Commit:  543c6efc0cef99a4d625da6633ca74d50dc9a9e3 
Dependencies:  Stopgaps: 
Description
Tachyon.plot
does not produce valid syntax for tachyon_rt
. The basic problem is that triangles are not printed correctly: they show as tuples instead. This means that the examples in the reference manual are broken.
Here is a minimal example:
sage: m = lambda u,v: u^2  v^3 sage: Q = Tachyon(camera_center=(5,0,4)) sage: Q.texture('t') sage: Q.light((20,20,40), 0.2, (1,1,1)) sage: Q.plot(m,(0,1),(0,1),'t',max_depth=1,initial_depth=1)
The string tachyon tries to render is this:
sage: print(Q.str()) begin_scene ... light center 20.0 20.0 40.0 rad 0.2 color 1.0 1.0 1.0 (1/2, 0, 1/4) (1/2, 1/2, 1/8) (1/4, 1/4, 3/64) t(1/2, 0, 1/4) (1/2, 1/2, 1/8) (3/4, 1/4, 35/64) t ... end_scene
Instead of 3tuples, we should see something like this:
... TRI V0 0.5 0.0 0.25 V1 0.5 0.5 0.125 V2 0.25 0.25 0.046875 t TRI V0 0.5 0.0 0.25 V1 0.5 0.5 0.125 V2 0.75 0.25 0.546875 t ...
Change History (14)
comment:1 Changed 5 years ago by
comment:2 Changed 5 years ago by
 Branch set to u/niles/16226/quickndirty
 Commit set to 3700e0d6af5515c309fef5d5d5f09f16ef7fba7e
comment:3 followup: ↓ 4 Changed 5 years ago by
 Cc kcrisman added
Don't worry about coordination on this ticket. Is this ready for review?
comment:4 in reply to: ↑ 3 Changed 5 years ago by
Replying to kcrisman:
Don't worry about coordination on this ticket. Is this ready for review?
Unfortunately I don't think so. First, it needs some doctests to have 100% coverage. Second, I think we need some better doctests for Tachyon.plot
and other functions, so that regressions like this are less likely in the future. Like maybe printing out a small portion of the tachyon string (what portion?), or its md5 hash (too fragile?), or checking the file size of the rendered image (too slow?).
Lastly, I'm not sure we really want to commit to __str__
being the same as .str()
. Another easy fix which doesn't force this would be to change the .str()
method of TrianglePlot
from using
[str(o) for o in self._objects]
to
[o.str() for o in self._objects]
and add a .str()
method for the default Triangle
and SmoothTriangle
classes. Since TrianglePlot
isn't used in the rest of Sage, this probably wouldn't cause too much trouble.
Moreover, this would be more consistent with what's done for all the other Tachyon types, and would avoid mixing up what .str()
means for Tachyon stuff.str()
is the exact code to be processedwith what __str__
means for Python stuff__str__
returns a userreadable string describing the object, but need not be as exact as __repr__
. Or maybe this mixup is inevitable and we might as well embrace it, even though it's confusing.
comment:5 followup: ↓ 6 Changed 5 years ago by
 Branch changed from u/niles/16226/quickndirty to u/niles/16226/nodblunderscore
 Commit changed from 3700e0d6af5515c309fef5d5d5f09f16ef7fba7e to 0b6e1184022052ef6437e7c5a0175561b4755ed7
Here's a different branch which entirely throws out __str__
and __repr__
for Tachyon objects. All methods are doctested, and all tests pass in sage/plot. I'm running (long) tests on the whole sage library now.
The plotting examples in tachyon.py are working as expected.
New commits:
1527571  don't use __str__ or __repr__ for tachyon string representations

af48ed4  fix doctests

0b6e118  more doctest fixes

comment:6 in reply to: ↑ 5 Changed 5 years ago by
Replying to niles:
I'm running (long) tests on the whole sage library now.
... aaaaand all tests pass!
Unless someone has a better idea, I can write something that prints the md5 digest of tachyon strings, and add some TEST docstrings that use it.
comment:7 Changed 5 years ago by
 Commit changed from 0b6e1184022052ef6437e7c5a0175561b4755ed7 to a2cb4232309afd5e084bb91a1e1410a1ce1f9603
Branch pushed to git repo; I updated commit sha1. New commits:
a2cb423  use verbose option to check that plots render successfully

comment:8 Changed 5 years ago by
 Status changed from new to needs_review
Ok, this is ready for review. I did try implementing a hash function, but this was even more fragile than I expected. Triangulations of surfaces can vary depending on floating point rounding and maybe other things too. So I just used .show(verbose=1)
in a few places to verify that the plots are processed without error.
Ideally, the raytracing function tachyon_rt
would throw an error if the raytracing is not successful, but that is a problem for another ticket: #16262.
comment:9 Changed 5 years ago by
 Milestone changed from sage6.2 to sage6.3
comment:10 Changed 5 years ago by
 Status changed from needs_review to needs_work
There seems to be a problem in one of str methods:
Doc claims to give
a b c da db dc color
but the result is [1, 2, 3] [2, 3, 4] [0, 0, 0] 0 [0, 0, 1] [0, 1, 0] [1, 0, 0]
which looks more like a b c color da db dc
comment:11 Changed 5 years ago by
 Commit changed from a2cb4232309afd5e084bb91a1e1410a1ce1f9603 to 543c6efc0cef99a4d625da6633ca74d50dc9a9e3
Branch pushed to git repo; I updated commit sha1. New commits:
543c6ef  correct docstring for str

comment:12 Changed 5 years ago by
 Status changed from needs_work to needs_review
good catch  fixed now
comment:13 Changed 5 years ago by
 Reviewers set to Frédéric Chapoton
 Status changed from needs_review to positive_review
Looks good to me. Positive review.
comment:14 Changed 5 years ago by
 Branch changed from u/niles/16226/nodblunderscore to 543c6efc0cef99a4d625da6633ca74d50dc9a9e3
 Resolution set to fixed
 Status changed from positive_review to closed
Tachyon plots use
sage.plot.plot3d.tri_plot.TrianglePlot
, which prints objects withstr(o)
(see tri_plot.py line 233).One quick fix is to add
__str__
or__repr__
methods toTachyonTriangle
andTachyonSmoothTriangle
which print appropriately. There may be a more robust fix though.