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: sage-6.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 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
...

Change History (14)

comment:1 Changed 5 years ago by 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.

comment:2 Changed 5 years ago by niles

  • Branch set to u/niles/16226/quick-n-dirty
  • Commit set to 3700e0d6af5515c309fef5d5d5f09f16ef7fba7e

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:

faac676quick fix
3700e0dquick fix for smooth triangles too

comment:3 follow-up: Changed 5 years ago by kcrisman

  • 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 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.

comment:5 follow-up: Changed 5 years ago by niles

  • Branch changed from u/niles/16226/quick-n-dirty to u/niles/16226/no-dbl-underscore
  • 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:

1527571don't use __str__ or __repr__ for tachyon string representations
af48ed4fix doctests
0b6e118more doctest fixes

comment:6 in reply to: ↑ 5 Changed 5 years ago by 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.

comment:7 Changed 5 years ago by git

  • Commit changed from 0b6e1184022052ef6437e7c5a0175561b4755ed7 to a2cb4232309afd5e084bb91a1e1410a1ce1f9603

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

a2cb423use verbose option to check that plots render successfully

comment:8 Changed 5 years ago by niles

  • 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 vbraun_spam

  • Milestone changed from sage-6.2 to sage-6.3

comment:10 Changed 5 years ago by chapoton

  • 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 git

  • Commit changed from a2cb4232309afd5e084bb91a1e1410a1ce1f9603 to 543c6efc0cef99a4d625da6633ca74d50dc9a9e3

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

543c6efcorrect docstring for str

comment:12 Changed 5 years ago by niles

  • Authors set to Niles Johnson
  • Status changed from needs_work to needs_review

good catch -- fixed now

comment:13 Changed 5 years ago by chapoton

  • 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 vbraun

  • Branch changed from u/niles/16226/no-dbl-underscore to 543c6efc0cef99a4d625da6633ca74d50dc9a9e3
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.