Opened 7 years ago
Closed 6 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 7 years ago by
comment:2 Changed 7 years ago by
- Branch set to u/niles/16226/quick-n-dirty
- Commit set to 3700e0d6af5515c309fef5d5d5f09f16ef7fba7e
comment:3 follow-up: ↓ 4 Changed 7 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 7 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 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: ↓ 6 Changed 7 years ago by
- 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:
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 7 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 7 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 7 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 7 years ago by
- Milestone changed from sage-6.2 to sage-6.3
comment:10 Changed 7 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 7 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 7 years ago by
- Status changed from needs_work to needs_review
good catch -- fixed now
comment:13 Changed 6 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 6 years ago by
- Branch changed from u/niles/16226/no-dbl-underscore 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.