Tachyon plot produces invalid file

[niles]

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 3-tuples, 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
...

[niles]

Tachyon plots use sage.plot.plot3d.tri_plot.TrianglePlot, which prints objects with str(o) (​see tri_plot.py line 233).

One quick fix is to add __str__ or __repr__ methods to TachyonTriangle and TachyonSmoothTriangle which print appropriately. There may be a more robust fix though.

[niles]

A quick and dirty fix which solves the problem at hand but ignores the larger problems of duplicated code and low coordination between Tachyon.plot and other surface plots in Sage.

---

New commits:

​faac676	quick fix
​3700e0d	quick fix for smooth triangles too

[kcrisman]

Don't worry about coordination on this ticket. Is this ready for review?

[niles]

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 processed---with what __str__ means for Python stuff---__str__ returns a user-readable 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.

[niles]

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

[niles]

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.

[git]

Branch pushed to git repo; I updated commit sha1. New commits:

​a2cb423

use verbose option to check that plots render successfully

[niles]

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.

[chapoton]

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

[git]

Branch pushed to git repo; I updated commit sha1. New commits:

​543c6ef

correct docstring for str

[niles]

good catch -- fixed now

[chapoton]

Looks good to me. Positive review.