Opened 7 years ago

Closed 3 years ago

#12212 closed defect (fixed)

Colormap for implicit_plot3d and parametric_plot3d

Reported by: niles Owned by: jason, was
Priority: blocker Milestone: sage-6.5
Component: graphics Keywords: colormap, plot
Cc: schilly, jason, jvkersch, kcrisman, ppurka, gutow Merged in:
Authors: Joris Vankerschaver, Frédéric Chapoton, Niles Johnson Reviewers: Frédéric Chapoton, Niles Johnson, Karl-Dieter Crisman, Jonathan Gutow
Report Upstream: N/A Work issues:
Branch: 36e4d8c (Commits) Commit: 36e4d8cae0037fc8442fbd20594a8e36c5b46196
Dependencies: #17645 Stopgaps:

Description (last modified by jdemeyer)

Colormaps work for plot3d, but not for implicit_plot3d or parametric_plot3d.

Compare the following:

sage: var('r v')
sage: cmsel = [colormaps['autumn'](i) for i in sxrange(0,1,0.05)]
sage: p = plot3d(0.2*(r**2 + v**2) + cos(2*r)*sin(2*v),(r,-2,2), (v,-2,2), adaptive=True, color=cmsel, plot_points=10, opacity=0.9)
sage: p2 = sphere((0,0,0),1,color='black',opacity=0.5)
sage: (p+p2).show(aspect_ratio=(1,1,1), figsize=[7,3])
sage: cmsel = [colormaps['autumn'](i) for i in sxrange(0,1,0.05)]
sage: var('x,y,z')
sage: implicit_plot3d(x^2+y^2+z^2==4, (x, -3, 3), (y, -3,3), (z, -3,3), color=cmsel)
ERROR: An unexpected error occurred while tokenizing input
The following traceback may be corrupted or invalid
The error message is: ('EOF in multi-line statement', (49, 0))
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
...
TypeError: 'NoneType' object is not iterable

See details at this Ask Sage question.

Upstream: http://chapoton.perso.math.cnrs.fr/jmol-14.2.11_2015.01.20.tar.bz2

Attachments (10)

implicit_plot_colormap.jpeg (24.6 KB) - added by jvkersch 7 years ago.
Implicit plot colormap (example from the ticket)
parametric_plot_colormap.jpeg (29.7 KB) - added by jvkersch 7 years ago.
parametric plot colormap
implicit_plot_nonuniform_colormap.jpeg (25.7 KB) - added by jvkersch 7 years ago.
implicit plot colormap (non-uniform)
spheres_opacity.jpeg (22.6 KB) - added by jvkersch 7 years ago.
partially translucent spheres
ellipsoid_curvature.jpeg (21.2 KB) - added by jvkersch 7 years ago.
ellipsoid colored by gaussian curvature
saddle_curvature.jpeg (25.3 KB) - added by jvkersch 7 years ago.
saddle colored by gaussian curvature
buckyball_curvature.jpeg (31.7 KB) - added by jvkersch 7 years ago.
complicated surface colored by gaussian curvature
resultat2.png (71.4 KB) - added by chapoton 4 years ago.
nice_shell.png (107.5 KB) - added by chapoton 4 years ago.
nice_shell-obj_1.pmesh (739.5 KB) - added by chapoton 4 years ago.
a jmol colored filed

Download all attachments as: .zip

Change History (178)

comment:1 Changed 7 years ago by kcrisman

  • Cc kcrisman added

comment:2 follow-up: Changed 7 years ago by jvkersch

I've noticed that the functionality for dealing with a color map can be found in the plot3d_adaptive method in sage.plot.plot3d.plot3d (notice the adaptive=True keyword in the above). This code only supposes that the graphical object is an instance of IndexFaceSet, and so it's not too hard to take out the code and apply it to parametric plots and implicit plots as well.

In fact, building upon the original idea one can apply color maps that vary not just in the z-direction, but according to the level sets of some arbitrary function. This is nice, for instance, if you want to paint a surface according to its curvature. And the original idea can also be adapted to gradually vary other texture attributes (for instance opacity) according to some scalar function.

I've put this functionality into a class tentatively called GradualTextureTransform, but since I'm not entirely happy with the class layout, I wanted to ask for your input before turning this into a full-fledged patch. I've put all of my code into a sagenb worksheet and I've also uploaded some sample images.

One issue with the worksheet is that the JMOL applet crashes (at least in my browser) after rendering a bunch of colormapped images. This could be because the colormap algorithm basically splits each surface into 10s or 100s of parts and colors those pieces individually, so having too many of these processed images around may be too much for the applet...

Changed 7 years ago by jvkersch

Implicit plot colormap (example from the ticket)

Changed 7 years ago by jvkersch

parametric plot colormap

Changed 7 years ago by jvkersch

implicit plot colormap (non-uniform)

Changed 7 years ago by jvkersch

partially translucent spheres

Changed 7 years ago by jvkersch

ellipsoid colored by gaussian curvature

Changed 7 years ago by jvkersch

saddle colored by gaussian curvature

Changed 7 years ago by jvkersch

complicated surface colored by gaussian curvature

comment:3 in reply to: ↑ 2 Changed 7 years ago by niles

Replying to jvkersch:

In fact, building upon the original idea one can apply color maps that vary not just in the z-direction, but according to the level sets of some arbitrary function.

This is great! Indeed my original interest was to color by some other function :)

I've put this functionality into a class tentatively called GradualTextureTransform, but since I'm not entirely happy with the class layout, I wanted to ask for your input before turning this into a full-fledged patch.

Thanks; I won't be able to look at this for a couple of weeks, but I'll let you know when I can.

comment:4 follow-up: Changed 5 years ago by lftabera

This is a really cool feature.

Joris, are you still working on this? The link is not valid. Could you attach the code hede?

comment:5 in reply to: ↑ 4 Changed 5 years ago by jvkersch

Joris, are you still working on this? The link is not valid. Could you attach the code hede?

Thanks for your comment; I totally forgot about this.

I've attached the worksheet here, feel free to play with it. I'd be all for making this into a patch if you want.

Edit: the worksheet is too big to serve as an attachment, so I'm providing this external link: http://www2.imperial.ac.uk/~jvankers/12212-implicit-colors-worksheet.sws

Last edited 5 years ago by jvkersch (previous) (diff)

comment:6 Changed 5 years ago by jdemeyer

  • Milestone changed from sage-5.11 to sage-5.12

comment:7 Changed 4 years ago by vbraun_spam

  • Milestone changed from sage-6.1 to sage-6.2

comment:8 Changed 4 years ago by kcrisman

comment:9 Changed 4 years ago by vbraun_spam

  • Milestone changed from sage-6.2 to sage-6.3

comment:10 Changed 4 years ago by chapoton

  • Branch set to u/chapoton/12212
  • Commit set to a6e0d47d681d64a5d0c21cceb1a1f012fba29645
  • Status changed from new to needs_review

Here is a git branch, made from the worksheet above. It needs a better integration into Sage, at the very least.


New commits:

a6e0d47trac #12212 allows to color implicit 3d plots

comment:11 follow-up: Changed 4 years ago by chapoton

  • Authors set to Joris Vankerschaver
  • Status changed from needs_review to needs_work

comment:12 in reply to: ↑ 11 Changed 4 years ago by niles

Replying to chapoton:

Thanks for getting this started. Could you clarify what you think the work issues are? Maybe we just need to set up the other plot commands to call this when they are given colormaps for the color argument?

Do you have any timings for applying these textures? Is the code reasonably fast?

comment:13 Changed 4 years ago by chapoton

Yes, I think it is necessary to make this available when sending a colormap to the color keyword of plot3d commands. How and where in the code to do this is not clear to me.

I have no idea about the speed and timings.

comment:14 Changed 4 years ago by jvkersch

When I wrote this, it was pretty memory-intensive, depending on the level function used, and could crash the notebook when I generated a bunch of successive plots. I didn't optimize anything for speed or efficiency, though.

comment:15 Changed 4 years ago by niles

I've played with this a little bit now, and it seems reasonably fast in my simple tests.

I realized that using an arbitrary scalar function (e.g. curvature) for the colormap only works for implicit surfaces, not parametrized surfaces. If I understand correctly, this is a fundamental limitation of this approach, because it applies colors only *after* the surface is constructed and therefore only has access to coordinates in 3-space, not the parametrization that was used to produce them.

This is still more useful than what currently exists (i.e., nothing!), but perhaps it can be integrated into the surface construction methods in a way that works for both implicit and parametrized surfaces.

comment:16 Changed 4 years ago by git

  • Commit changed from a6e0d47d681d64a5d0c21cceb1a1f012fba29645 to b6ba287bc4c7cac2fbbc4c274ac351dc7b6e37af

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

1cca14bMerge branch 'u/chapoton/12212' of ssh://trac.sagemath.org:22/sage into 12212
b6ba287trac #12212 introduce colored faces in IndexFaceSet + tachyon view

comment:17 Changed 4 years ago by chapoton

Hello,

I have worked hard, and managed to modify the class IndexFaceSet?, so that it can handle colored faces (one face per color). One can now give one more parameter to IndexFaceSet?, which is a texture list. This is only preliminary, as this new behaviour can only be viewed with tachyon, and from the texture only the color is used.

Is there somebody familiar with jmol syntax and wanting to help ?

comment:18 Changed 4 years ago by git

  • Commit changed from b6ba287bc4c7cac2fbbc4c274ac351dc7b6e37af to b663f1382e9274e77772534f63b2cace8e33f611

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

b663f13trac #12212 X3D output for colored IndexedFaceSet that seems to work

comment:19 Changed 4 years ago by git

  • Commit changed from b663f1382e9274e77772534f63b2cace8e33f611 to 94239a4d70ad7fdac414cb15af7db746d77387a0

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

94239a4trac #12212 more doc, and all tests pass

comment:20 Changed 4 years ago by git

  • Commit changed from 94239a4d70ad7fdac414cb15af7db746d77387a0 to edb4c5a4211e9920a95fe5e1e166a3e7a997e958

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

edb4c5atrac #12212 incorporate coloring into parametric_surface

Changed 4 years ago by chapoton

comment:21 follow-up: Changed 4 years ago by chapoton

I have introduced a new argument for a pair (coloring function, colormap) in parametric plot. This can produce that (in tachyon and x3d only for the moment):

Last edited 4 years ago by chapoton (previous) (diff)

comment:22 Changed 4 years ago by chapoton

After reading the doc of jmol here

http://chemapps.stolaf.edu/jmol/docs/#isosurfacesurfaceobject--surfacedata-derivedisosurfaces

I am rather pessimistic about achieving to transmit colored surfaces to jmol. Currently, we send pmesh files to jmol. Using this format, there seems to be no way to give a color to every face (other than horrible splitting : one pmesh file for every color).

One other possibility would be to use obj files instead. But jmol is using the obj format in a strange (bad?) way : colors are coded in the names of the groups, instead of using the material mtl file. To do this, one would need to group faces according to their colors, which would take some work to code.

This same work would probably also be necessary just to make sure that the obj file output works correctly for colored surfaces.

I have not yet understood if the json format allows to deal eeasily with colored surfaces.

comment:23 in reply to: ↑ 21 Changed 4 years ago by niles

Replying to chapoton:

I have introduced a new argument for a pair (coloring function, colormap) in parametric plot. This can produce that (in tachyon and x3d only for the moment):

This is fantastic!! I'm excited to try it out :)

I wouldn't worry too much about jmol -- I think that eventually Sage will transition to another interactive 3d viewer since java support is declining anyway. For example, I believe sagemath cloud uses three.js.

Last edited 4 years ago by niles (previous) (diff)

comment:24 Changed 4 years ago by kcrisman

Does jsmol support something different? (I assume not.) As long as the documentation says you need one of the other things to see this, that is okay.

comment:25 Changed 4 years ago by git

  • Commit changed from edb4c5a4211e9920a95fe5e1e166a3e7a997e958 to 9e53e2e88b370cf0edd784c1da73ce529173f362

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

9e53e2etrac #12212 modified the json format to allow for colored faces

comment:26 Changed 4 years ago by chapoton

Hello,

I have still not taken care of the jmol issue, which seems to be hard without changing jmol itself.

I have changed the json output in IndexFaceSet?, which is a private format used in the notebook for the viewer 'canvas3d'. I have also proposed the corresponding changes on sagenb.

comment:27 Changed 4 years ago by ppurka

  • Cc ppurka added

comment:28 Changed 4 years ago by git

  • Commit changed from 9e53e2e88b370cf0edd784c1da73ce529173f362 to c06dcfae7dd9aa796eb833eb422024af19dbc7b6

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

c06dcfatrac #12212 trying to use p3 .format ; little more doc

comment:29 follow-up: Changed 4 years ago by kcrisman

You're going to need some extra blank lines for this to format right.

         [[(1.0, 0.0, 0.0), (0.0, 1.0, 0.0), (0.0, 0.0, 0.0)], [(1.0, 0.0, 0.0), (0.0, 1.0, 0.0), (0.0, 0.0, 1.0)], [(1.0, 0.0, 0.0), (0.0, 1.0, 0.0), (0.0, 0.0, 2.0)], [(1.0, 0.0, 0.0), (0.0, 1.0, 0.0), (0.0, 0.0, 3.0)], [(1.0, 0.0, 0.0), (0.0, 1.0, 0.0), (0.0, 0.0, 4.0)], [(1.0, 0.0, 0.0), (0.0, 1.0, 0.0), (0.0, 0.0, 5.0)], [(1.0, 0.0, 0.0), (0.0, 1.0, 0.0), (0.0, 0.0, 6.0)], [(1.0, 0.0, 0.0), (0.0, 1.0, 0.0), (0.0, 0.0, 7.0)]]
         sage: S.show()
-    Example of colored IndexFaceSet (:trac:`12212`)::
+    A simple example of colored IndexFaceSet (:trac:`12212`)::
         sage: from sage.plot.plot3d.index_face_set import IndexFaceSet
         sage: from sage.plot.plot3d.texture import Texture

Should be

         [[(1.0, 0.0, 0.0), (0.0, 1.0, 0.0), (0.0, 0.0, 0.0)], [(1.0, 0.0, 0.0), (0.0, 1.0, 0.0), (0.0, 0.0, 1.0)], [(1.0, 0.0, 0.0), (0.0, 1.0, 0.0), (0.0, 0.0, 2.0)], [(1.0, 0.0, 0.0), (0.0, 1.0, 0.0), (0.0, 0.0, 3.0)], [(1.0, 0.0, 0.0), (0.0, 1.0, 0.0), (0.0, 0.0, 4.0)], [(1.0, 0.0, 0.0), (0.0, 1.0, 0.0), (0.0, 0.0, 5.0)], [(1.0, 0.0, 0.0), (0.0, 1.0, 0.0), (0.0, 0.0, 6.0)], [(1.0, 0.0, 0.0), (0.0, 1.0, 0.0), (0.0, 0.0, 7.0)]]
         sage: S.show()
+
-    Example of colored IndexFaceSet (:trac:`12212`)::
+    A simple example of colored IndexFaceSet (:trac:`12212`)::
+
         sage: from sage.plot.plot3d.index_face_set import IndexFaceSet
         sage: from sage.plot.plot3d.texture import Texture

comment:30 Changed 4 years ago by git

  • Commit changed from c06dcfae7dd9aa796eb833eb422024af19dbc7b6 to db88a1fd27a1f72e1e10a4f0e02699671d78098b

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

db88a1ftrac #12212 including index_face_set in the reference + more doc

comment:31 Changed 4 years ago by git

  • Commit changed from db88a1fd27a1f72e1e10a4f0e02699671d78098b to edc63704fb1f82ec1897ec299935b2589dfb2170

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

edc6370trac #12212 correct one failing doctest

comment:32 follow-up: Changed 4 years ago by chapoton

The 'canvas3d' viewer (using json output with colors defined here and a patched sagenb) results look good, but there seems to be a little problem in the notebook. Here is one example

from sage.plot.plot3d.parametric_surface import ParametricSurface
cm = colormaps.autumn
def c(x,y): return sin(x*y)**2
def g(x,y): return x, y, x**2 + y**2
P1 = ParametricSurface(g, (srange(-10,10,0.1), srange(-5,5.0,0.1)),(c,cm))

When one defines several parametric surfaces, say P1 and P2. Try P1.show(viewer='canvas3d') inside one cell. Then change this cell to P2.show(viewer='canvas3d'). It will still show P1 and will never change, whatever you do to P1. No such problem inside a new cell. Seems to be some caching problem.

comment:33 Changed 4 years ago by kcrisman

  • Authors changed from Joris Vankerschaver to Joris Vankerschaver, Frederic Chapoton

Good point, but from my testing this is completely independent of your changes, so shouldn't hold this up. Probably it's because the notebook displays whatever files it finds, but in this case the previous one is likely not destroyed or perhaps it's numbered differently or something. Can you open an issue there about this (assuming there isn't already one open here on Trac or there on github)?

Oh, and if I missed an accent in your name my apologies!

Last edited 4 years ago by kcrisman (previous) (diff)

comment:34 Changed 4 years ago by git

  • Commit changed from edc63704fb1f82ec1897ec299935b2589dfb2170 to a7302cbc80856a6a707ac4315e5e7b5bc451ecb1

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

a7302cbtrac #12212 polishing doc and correction to mobius band

comment:35 Changed 4 years ago by git

  • Commit changed from a7302cbc80856a6a707ac4315e5e7b5bc451ecb1 to 0af95c7f0727a4e7f39391952fa58ce22fe01f9a

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

0af95c7trac #12212 more doc, also in parametric_plot3d.py

comment:36 in reply to: ↑ 29 Changed 4 years ago by kcrisman

You're going to need some extra blank lines for this to format right.

Your recent changes still don't fix this, as far as I can tell. Try building the documentation.

         [[(1.0, 0.0, 0.0), (0.0, 1.0, 0.0), (0.0, 0.0, 0.0)], [(1.0, 0.0, 0.0), (0.0, 1.0, 0.0), (0.0, 0.0, 1.0)], [(1.0, 0.0, 0.0), (0.0, 1.0, 0.0), (0.0, 0.0, 2.0)], [(1.0, 0.0, 0.0), (0.0, 1.0, 0.0), (0.0, 0.0, 3.0)], [(1.0, 0.0, 0.0), (0.0, 1.0, 0.0), (0.0, 0.0, 4.0)], [(1.0, 0.0, 0.0), (0.0, 1.0, 0.0), (0.0, 0.0, 5.0)], [(1.0, 0.0, 0.0), (0.0, 1.0, 0.0), (0.0, 0.0, 6.0)], [(1.0, 0.0, 0.0), (0.0, 1.0, 0.0), (0.0, 0.0, 7.0)]]
         sage: S.show()
-    Example of colored IndexFaceSet (:trac:`12212`)::
+    A simple example of colored IndexFaceSet (:trac:`12212`)::
         sage: from sage.plot.plot3d.index_face_set import IndexFaceSet
         sage: from sage.plot.plot3d.texture import Texture

Should be

         [[(1.0, 0.0, 0.0), (0.0, 1.0, 0.0), (0.0, 0.0, 0.0)], [(1.0, 0.0, 0.0), (0.0, 1.0, 0.0), (0.0, 0.0, 1.0)], [(1.0, 0.0, 0.0), (0.0, 1.0, 0.0), (0.0, 0.0, 2.0)], [(1.0, 0.0, 0.0), (0.0, 1.0, 0.0), (0.0, 0.0, 3.0)], [(1.0, 0.0, 0.0), (0.0, 1.0, 0.0), (0.0, 0.0, 4.0)], [(1.0, 0.0, 0.0), (0.0, 1.0, 0.0), (0.0, 0.0, 5.0)], [(1.0, 0.0, 0.0), (0.0, 1.0, 0.0), (0.0, 0.0, 6.0)], [(1.0, 0.0, 0.0), (0.0, 1.0, 0.0), (0.0, 0.0, 7.0)]]
         sage: S.show()
+
-    Example of colored IndexFaceSet (:trac:`12212`)::
+    A simple example of colored IndexFaceSet (:trac:`12212`)::
+
         sage: from sage.plot.plot3d.index_face_set import IndexFaceSet
         sage: from sage.plot.plot3d.texture import Texture

comment:37 follow-up: Changed 4 years ago by chapoton

I have tried, and it builds. Please do look at the code and not only at the diffs. Or maybe just click on the green link for the branch.

comment:38 in reply to: ↑ 37 Changed 4 years ago by kcrisman

I have tried, and it builds. Please do look at the code and not only at the diffs. Or maybe just click on the green link for the branch.

Thanks for this pointer; I find that on very large tickets it pays just to look at diffs so seeing the changes is easy. Interestingly, now the diffs look correct as well - they definitely didn't before - so I am confused! So I'm quite sorry for the noise.

On a related note, I would recommend adding something about Jmol not working with this in "main" documentation, like

+       One can use the keyword ``colordata`` to color the surface using a
+       coloring function and a colormap::

or

+One can instead provide a coloring function and a color map::
+

Also, do you want to include src/sage/plot/texture_map.py in the reference manual like you've added index_face_set, or is that all internals better left out? You (or the original author) spent a lot of time on it so I'm sure it's fine if you do. (Or did I screw up again and it is actually there? I think I didn't miss it...)

comment:39 Changed 4 years ago by git

  • Commit changed from 0af95c7f0727a4e7f39391952fa58ce22fe01f9a to 10e1a7af46b4fca28339970c51688e0026e1ba47

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

10e1a7atrac #12212 adding texture_map to the doc, plus relocating it in plot3d

comment:40 follow-up: Changed 4 years ago by chapoton

  • Authors changed from Joris Vankerschaver, Frederic Chapoton to Joris Vankerschaver, Frédéric Chapoton

Hello,

I have polished the doc a little bit more, and included texture_map in the doc. In my opinion this is getting closer to the needs_review status. It is already quite a big "patch".

comment:41 Changed 4 years ago by kcrisman

True. What would you say still needs review (e.g., which commits)? Remember, you yourself can review jvkersch's contributions.

comment:42 in reply to: ↑ 40 ; follow-up: Changed 4 years ago by niles

Replying to chapoton:

Hello,

I have polished the doc a little bit more, and included texture_map in the doc. In my opinion this is getting closer to the needs_review status. It is already quite a big "patch".

Thanks for all the work here -- I've been testing it too and I'm really impressed with the progress! Here are some things that I think still need to be addressed:

  • I think there need to be some more doctests that verify the coloring of triangles is working. At the moment, if some bug stopped colors from being applied to triangles, I think most of the doctests would still pass. Maybe at least you could add tests in IndexFaceSet for tachyon_repr, obj, and x3d_geometry similar to the one for json_repr.
  • Was IndexFaceSet.dual broken before this patch? Is there another ticket open for it?
  • The argument texture_list for IndexFaceSet.__init__ displaces the argument enclosed, which technically means that the new behavior is not backwards-compatible. I'm sorry to bring this up, because I think it's a really annoying detail, but I think Sage policy is that there should be a deprecation period in case someone somewhere is using S = IndexFaceSet(face_list, point_list, False). One thing would be to add a check `if type(texture_list) is bool:..." which issues a deprecation warning and fixes the arguments. Or, maybe better, put the texture argument after the enclosure one, and always use it as a keyword.
  • Since colormaps don't work with jmol, I think the demonstrations in parametric_plot3d and elsewhere should specifiy a viewer where they do work. For example:
sage: u,v = var('u,v')
sage: col = rainbow(10, 'rgbtuple')
sage: def cf(u,v): return sin(u^2 + v^2)**2
sage: parametric_plot3d((u, u+v, v), (u, -3,3), (v, -3, 3), colordata=(cf, colormaps.PiYG), viewer='tachyon')
  • Why do this example and some others look so bad? I think the first examples (the ones users are most likely to see) should be chosen to look really good. If specifying a large number of plot points and marking #long or #optional is necessary, then I think that's better than fast but low-quality examples. (Fast, low-quality examples are good for automatic testing, in parts of the documentation that new users are not likely to start with.)
  • The syntax for colormaps of plotted functions (e.g. with plot3d) and parametric surfaces should be the same if at all possible. Since plot3d calls parametric_plot3d, they should use the same code for colormaps. Also check plot3d_adaptive to see what it's doing.
  • In parametric_plot3d, wouldn't it be more consistent to use color_data for the argument name, rather than colordata? I know colormap is an established argument name (coming from matplot lib, I think), but I think it's better to use Sage's convention if possible.
  • ImplicitSurface is derived from IndexFaceSet, so it should be reasonably straightforward to get colormaps for implicit plots working too. Is there a good way to do it without just duplicating the code from ParametricSurface? There seems to already be some duplicated code between them. Maybe there needs to be some other kind of PlotSurface class that contains the common code?

comment:43 Changed 4 years ago by git

  • Commit changed from 10e1a7af46b4fca28339970c51688e0026e1ba47 to eb4be95baaf9dc4baa3587cc2f9a4f6a8b8cdd02

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

17bed30trac #12212 improved examples, more doc (after reviewers comments)
eb4be95trac #12212 better use of colormaps in texture_map

comment:44 in reply to: ↑ 42 Changed 4 years ago by chapoton

Thanks for your comments. I have adressed some of them

  • I think there need to be some more doctests that verify the coloring of triangles is working. At the moment, if some bug stopped colors from being applied to triangles, I think most of the doctests would still pass. Maybe at least you could add tests in IndexFaceSet for tachyon_repr, obj, and x3d_geometry similar to the one for json_repr.

I have added a few doctests in index_face_set

  • Was IndexFaceSet.dual broken before this patch? Is there another ticket open for it?

It is not really broken, just a bit fragile. I put a working doctest.

  • The argument texture_list for IndexFaceSet.__init__ displaces the argument enclosed, which technically means that the new behavior is not backwards-compatible. I'm sorry to bring this up, because I think it's a really annoying detail, but I think Sage policy is that there should be a deprecation period in case someone somewhere is using S = IndexFaceSet(face_list, point_list, False). One thing would be to add a check `if type(texture_list) is bool:..." which issues a deprecation warning and fixes the arguments. Or, maybe better, put the texture argument after the enclosure one, and always use it as a keyword.

I have changed the order to keep "enclosed" before "texture_list"

  • Since colormaps don't work with jmol, I think the demonstrations in parametric_plot3d and elsewhere should specifiy a viewer where they do work. For example:
sage: u,v = var('u,v')
sage: col = rainbow(10, 'rgbtuple')
sage: def cf(u,v): return sin(u^2 + v^2)**2
sage: parametric_plot3d((u, u+v, v), (u, -3,3), (v, -3, 3), colordata=(cf, colormaps.PiYG), viewer='tachyon')

I have added lines with P.show(viewer='tachyon') where appropriate

  • Why do this example and some others look so bad? I think the first examples (the ones users are most likely to see) should be chosen to look really good. If specifying a large number of plot points and marking #long or #optional is necessary, then I think that's better than fast but low-quality examples. (Fast, low-quality examples are good for automatic testing, in parts of the documentation that new users are not likely to start with.)

I have tried to enhance the quality of examples

  • The syntax for colormaps of plotted functions (e.g. with plot3d) and parametric surfaces should be the same if at all possible. Since plot3d calls parametric_plot3d, they should use the same code for colormaps. Also check plot3d_adaptive to see what it's doing.

This is a difficult point. Achieving coeherence would be great, but I do not think that I am able to do that. For another ticket ?

  • In parametric_plot3d, wouldn't it be more consistent to use color_data for the argument name, rather than colordata? I know colormap is an established argument name (coming from matplot lib, I think), but I think it's better to use Sage's convention if possible.

I have switched to color_data

  • ImplicitSurface is derived from IndexFaceSet, so it should be reasonably straightforward to get colormaps for implicit plots working too. Is there a good way to do it without just duplicating the code from ParametricSurface? There seems to already be some duplicated code between them. Maybe there needs to be some other kind of PlotSurface class that contains the common code?

This is not as straightforward as it may look. It would be great to do that, but the code for implicit plot is very sophisticated, and it is not easy to see where to put the color assignment of faces.

PLUS:

I have reformatted texture_map so that the use of colormaps is simpler.

comment:45 Changed 4 years ago by git

  • Commit changed from eb4be95baaf9dc4baa3587cc2f9a4f6a8b8cdd02 to c01c2bc6575aa6324233dd0a84949cdc9f6e0188

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

c01c2bctrac #12212 now color_data works also for implicit plots

comment:46 follow-up: Changed 4 years ago by chapoton

I have managed (it was a bit painful) to make it work also for implicit surfaces. It needs to be doctested, but it works pretty well.

comment:47 Changed 4 years ago by git

  • Commit changed from c01c2bc6575aa6324233dd0a84949cdc9f6e0188 to 2b3b80cc17528bcdd60d6ee1344de808604ac437

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

110abe6Merge branch 'u/chapoton/12212' of ssh://trac.sagemath.org:22/sage into 12212
2b3b80ctrac #12212 preparing the ground for color in jmol

comment:48 follow-up: Changed 4 years ago by chapoton

I have contacted the Jmol team to know if they could write code to import some colored pmesh file. They have reacted positively. To be continued.

comment:49 Changed 4 years ago by git

  • Commit changed from 2b3b80cc17528bcdd60d6ee1344de808604ac437 to fc1f730aa242792cd3748fb64a90692a56a4a6d9

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

fc1f730trac #12212 final preparation for Jmol, plus cross links in doc

comment:50 Changed 4 years ago by git

  • Commit changed from fc1f730aa242792cd3748fb64a90692a56a4a6d9 to 1f7fc206043fb1ba781b6355cff553276821edeb

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

1f7fc20trac #12212 trying to enhance doctest coverage

Changed 4 years ago by chapoton

comment:51 Changed 4 years ago by chapoton

Here is what Jmol will be able to get from sage :

comment:52 Changed 4 years ago by vbraun_spam

  • Milestone changed from sage-6.3 to sage-6.4

comment:53 in reply to: ↑ 48 Changed 4 years ago by kcrisman

I have contacted the Jmol team to know if they could write code to import some colored pmesh file. They have reacted positively. To be continued.

Can you post a link to that discussion, if it is on a world-readable list? Just in case it is, if you contacted privately of course there is no need to do so.

comment:54 Changed 4 years ago by chapoton

This was a private mail exchange. The conclusion by Bob Hanson is

Later this week it will appear in Jmol 14.2.5.

So it would be great to have #16003 and #16004 use this Jmol version.

comment:55 Changed 4 years ago by kcrisman

  • Cc gutow added

Wow, that is awesome! I assume jsmol would use this as well... Are you able to review those changes to at least some extent as well? Jonathan does a very thorough job with them but they do still need review, and I always seem to have trouble really setting things up right - also review using Linux and its browsers would be helpful, as I only have Mac access. Now that we are in the 6.4 cycle this is a perfect time.

comment:56 in reply to: ↑ 46 ; follow-up: Changed 4 years ago by niles

Replying to chapoton:

I have managed (it was a bit painful) to make it work also for implicit surfaces. It needs to be doctested, but it works pretty well.

Great!! I think there is some codesmell here though, both in duplicated code and in the inconsistent interface. Constant color is a special case of color maps, so there shouldn't be a need to treat the two cases separately -- in other words, the global_texture flag should be unnecessary. In any kind of plot, one can give the color argument as a string, a tuple, a Color object, or perhaps other things -- it makes more sense that one should also be able to give the colormap data to that argument. The type of colormap should be determined there (in the Texture class, or some other unified place) and then handed as a Texture subclass to IndexFaceSet.

comment:57 in reply to: ↑ 56 ; follow-up: Changed 4 years ago by kcrisman

  • Reviewers set to Frédéric Chapoton, Niles Johnson

I have managed (it was a bit painful) to make it work also for implicit surfaces. It needs to be doctested, but it works pretty well.

Great!! I think there is some codesmell here though, both in duplicated code and in the inconsistent interface. Constant color is a special case of color maps, so there shouldn't be a need to treat the two cases separately -- in other words, the global_texture flag should be unnecessary. In any kind of plot, one can give the color argument as a string, a tuple, a Color object, or perhaps other things -- it makes more sense that one should also be able to give the colormap data to that argument. The type of colormap should be determined there (in the Texture class, or some other unified place) and then handed as a Texture subclass to IndexFaceSet.

Niles, is this easy for you to implement? I don't want to have this held up unduly, though you are right that the interface should be consistent for colors.

Let me see what I see as the status here:

  • Needs chapoton to verify what he has already positively reviewed. (That is presumably everything he didn't write.)
  • Niles can mention if this is the only thing he is worried about, or whether other changes still need review (or doctests needed), etc.
  • I'm surprised that something like float_to_integer doesn't already exist, I recall we used some very similar hacks before. Huh.
  • Leave jmol incorporation to another ticket, now that I see the commits that "set up" jmol don't actually require it - this will allow us to get this in without

comment:58 in reply to: ↑ 57 Changed 4 years ago by niles

Replying to kcrisman:

I think there is some codesmell here though, both in duplicated code and in the inconsistent interface.

The type of colormap should be determined there (in the Texture class, or some other unified place) and then handed as a Texture subclass to IndexFaceSet.

Niles, is this easy for you to implement? I don't want to have this held up unduly, though you are right that the interface should be consistent for colors.

Unfortunately, no, I don't think this will be easy. This ticket has been open for 3 years, and there's been an amazing amount of progress in the last month, but I think we need to be patient and make sure that when we establish new functionality we do so in a way that is carefully-considered and well-polished. Otherwise we'll be stuck maintaining a difficult system. (Remember that standard deprecation time is one year, so a non-backwards-compatible improvement to the interface would be slow-going.)

comment:59 follow-ups: Changed 4 years ago by kcrisman

Thanks, Niles. I didn't realize there was anything backwards-incompatible here, just additional keywords, but I take your point.

Unrelated and very minor note: in parametric_plot3d.py there is a superfluous variable declaration

+           sage: u,v = var('u,v')

which should be removed.

comment:60 Changed 4 years ago by git

  • Commit changed from 1f7fc206043fb1ba781b6355cff553276821edeb to 0ae10f534826c0addaf4d957bf7c0586ba0a88c4

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

92d9e5bMerge branch 'u/chapoton/12212' of ssh://trac.sagemath.org:22/sage into 12212
0ae10f5trac #12212 keeping the color keyword in json method

comment:61 in reply to: ↑ 59 Changed 4 years ago by kcrisman

Thanks, Niles. I didn't realize there was anything backwards-incompatible here, just additional keywords, but I take your point.

Niles, was the color keyword in json what you were referring to, or something else?

comment:62 Changed 4 years ago by chapoton

Please note that the last change I made is not about a keyword in the python method json, but a change about a key in the dict that is passed to canvas3D.

I understand that the interface needs to be polished. Nevertheless I am starting to get tired of that stuff, and I will get busy with lots of others things in the next months. So if you feel like improving things, please proceed.

By the way, there still remains the issue of what happens in sagemath cloud. I have just asked a question on sage-devel about that.

comment:63 in reply to: ↑ 59 Changed 4 years ago by kcrisman

Unrelated and very minor note: in parametric_plot3d.py there is a superfluous variable declaration

+           sage: u,v = var('u,v')

which should be removed.

And this comment of mine is apparently wrong too! Huh.

comment:64 in reply to: ↑ 59 ; follow-up: Changed 4 years ago by niles

Replying to kcrisman:

Thanks, Niles. I didn't realize there was anything backwards-incompatible here, just additional keywords,

I think I was unclear -- sorry. As I understand it, the code here isn't backward incompatible. What I meant is that the uniform interface I envision probably isn't compatible with the current interface. So the idea to release now, fix later (which I generally support!) would mean that we can't "fix" until after a year of deprecation.

And I think this is about more than interface -- I think the code may have to be reorganized, with some methods moved to different classes, and there would need to be deprecation for that as well.

I'm really frustrated about this too -- I'd hate to see this rot for another 3 years -- but I just can't convince myself it's good to release this as is. Do you disagree?

comment:65 in reply to: ↑ 64 Changed 4 years ago by kcrisman

I think I was unclear -- sorry. As I understand it, the code here isn't backward incompatible. What I meant is that the uniform interface I envision probably isn't compatible with the current interface. So the idea to release now, fix later (which I generally support!) would mean that we can't "fix" until after a year of deprecation.

And I think this is about more than interface -- I think the code may have to be reorganized, with some methods moved to different classes, and there would need to be deprecation for that as well.

I'm really frustrated about this too -- I'd hate to see this rot for another 3 years -- but I just can't convince myself it's good to release this as is. Do you disagree?

I guess I'm not clear about what would need to be deprecated. Remember, it's only user-visible things that would need this. If it's backend stuff... Maybe I just need more explicit detail about what your proposed reorganization would imply for the user interface. texture_map.py seems okay, it's perhaps mainly the colordata tuple you are concerned with? I haven't thought very deeply at all about this, I know your experience with Tachyon has given you a lot of chances to do so.

comment:66 follow-up: Changed 4 years ago by niles

Yeah, mainly the colordata tuple, but I think it's a symptom of some deeper organization issues.

But maybe I'm just being squeamish because I haven't thought hard enough about this. I propose one of the following:

  • You or Frédéric convince me the interface can be made uniform without any difficult reorganization (hopefully by doing it :)
  • I figure it out for myself, after some hard thinking (probably slowly :(

comment:67 in reply to: ↑ 66 Changed 4 years ago by kcrisman

I think I agree now that there is no real reason for the global texture thing, there should be a way to just use that color if it's available.

Yeah, mainly the colordata tuple, but I think it's a symptom of some deeper organization issues.

I think I dug up another one of your qualms next.

If I understand correctly, this is a fundamental limitation of this approach, because it applies colors only *after* the surface is constructed and therefore only has access to coordinates in 3-space, not the parametrization that was used to produce them.

But I don't know that these are insurmountable. I mean, how else could one give not just a cmap/color, but also a way to actually allocate it? I guess one could require using it post-hoc like in texture_map.py but I don't think that is necessary.

Also, I have now actually read through a lot more of the code as a whole. I see three things.

  • Awesome added (needed) documentation and cleanup everywhere, very nice
  • Joris' original texture map file, which seems to be used nowhere else and allows one to apply such a transform after creating a graphics object
  • chapoton's impressive direct additions of this functionality to things while creating the object

comment:68 Changed 4 years ago by git

  • Commit changed from 0ae10f534826c0addaf4d957bf7c0586ba0a88c4 to e28acd51f446f4a47aa73a7019c59b9f6bd148d1

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

e28acd5Merge branch with 6.4.beta3

comment:69 Changed 4 years ago by git

  • Commit changed from e28acd51f446f4a47aa73a7019c59b9f6bd148d1 to 41fe504cbf6dc5244e22bfd6dd4b374fd7d1eb28

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

41fe504Merge with 6.4.beta4

comment:70 follow-up: Changed 4 years ago by chapoton

Sorry, but I really do not see what I could do to improve things here.

The Texture class is there to handle a single texture. It makes no sense to feed it with a colormap. So it is not possible to use the color keyword for a colormap, because the color keyword is handled by the Texture class.

I really think that the global_texture dichotomy makes sense, as it is not at all the same thing to use a single Texture or one Texture for every vertex.

Maybe one could make something like a class TextureValuedFunction, but this seems a bit heavy to me.

I would like to put this ticket back to needs review.

comment:71 Changed 4 years ago by kcrisman

Okay, I've been trying to think about this, because I just don't think this is necessarily going to resolve itself easily. What if instead of color_data being a named argument it became (yet another) keyword? Then in principle that could (later) be overloaded with the ability to give color based on the parametrization, not the embedding in space, since that would probably need more than the one function. One would have then the following (potential) options:

  • Take an existing 3d plot, and apply one of Joris' transformations to it as a function of x,y,z.
  • Have a colormap applied at the time of creation via one of Frederic's functions of x,y,z.
  • Have a colormap applied at the time of parametrization, which would then be a color based on a function of the parameters, perhaps with some check for variable names or whatnot. (The previous options already cover based on implicit plots.) Or it could just have a different form, I don't know.

Another option would be to have the color keyword take a colormap, but then raise an error if an optional keyword color_function was not supplied.

comment:72 in reply to: ↑ 32 Changed 4 years ago by kcrisman

The 'canvas3d' viewer (using json output with colors defined here and a patched sagenb) results look good, but there seems to be a little problem in the notebook. Here is one example When one defines several parametric surfaces, say P1 and P2. Try P1.show(viewer='canvas3d') inside one cell. Then change this cell to P2.show(viewer='canvas3d'). It will still show P1 and will never change, whatever you do to P1. No such problem inside a new cell. Seems to be some caching problem.

Correct, this is #11275 which for some reason I didn't report upstream when I realized it a few days ago.

comment:73 in reply to: ↑ 70 ; follow-up: Changed 4 years ago by niles

Replying to chapoton:

Sorry, but I really do not see what I could do to improve things here.

The Texture class is there to handle a single texture.

...

I really think that the global_texture dichotomy makes sense, as it is not at all the same thing to use a single Texture or one Texture for every vertex.

Ah -- this must be one source of our miscommunication. As I understand it, a texture is a color-valued function on a surface. So I guess what you mean by a single texture is what I would call a *constant* texture. And I don't think it would be heavy to make a class which treats textures in the more general sense. The code for this is, as I recall, already existing in what you've written, but has been duplicated in various places because there isn't a single class for it.

comment:74 in reply to: ↑ 73 ; follow-up: Changed 4 years ago by kcrisman

Ah -- this must be one source of our miscommunication. As I understand it, a texture is a color-valued function on a surface. So I guess what you mean by a single texture is what I would call a *constant* texture. And I don't think it would be heavy to make a class which treats textures in the more general sense. The code for this is, as I recall, already existing in what you've written, but has been duplicated in various places because there isn't a single class for it.

Now I'm confused! But I guess that's okay, if that helps resolve things.

comment:75 in reply to: ↑ 74 Changed 4 years ago by niles

Replying to kcrisman:

Now I'm confused! But I guess that's okay, if that helps resolve things.

Ha! Well, I hope to get some time on this in the next few weeks.

comment:76 Changed 4 years ago by kcrisman

Here's another weird thing - we have this whole plot3d_adaptive stuff. Not necessarily needed to discuss here, but strange that it has fairly different arguments than the rest - including weird automatic color stuff.

comment:77 follow-up: Changed 4 years ago by niles

I've thought more about this now. First, here are some observations:

  • plot3d_adaptive, TrianglePlot, and the rest of tri_plot.py
    • This code is disjoint from the rest of the plotting code and needs to be integrated at some point in the future.
    • Makes use of IndexFaceSet.partition, but that method isn't used anywhere else, except now in GradualTextureTransform
  • Each face in an IndexFaceSet has a color attribute, but no other texture attributes (namely opacity, but also ambient, diffuse, specular)
    • The color_data argument, as it currently stands, directly sets this color attribute for each face as its constructed in ParametricSurface.triangulate or ImplicitSurface.triangulate
    • The two different triangulation methods differ substantially, and therefore probably shouldn't be unified
    • This approach (adding data to triangles) can only set color values, not other texture values, unless IndexFaceSet is expanded
    • I believe this is also connected to the problem of viewing with jmol, but I'm not completely sure.
    • Surfaces constructed with plot3d_adaptive are triangulated with a different codebase (tri_plot.py) and then converted to IndexFaceSet objects.
  • IndexFaceSet is a subclass of PrimitiveObject (as in graphics primitive), and hence has support for arbitrary texture information via PrimitiveObject.set_texture.
    • This sets the same texture information for *all* faces in a given IndexFaceSet instance.
    • The code in plot3d_adaptive, and hence also in GradualTextureTransform which copies it, makes a different instance of IndexFaceSet for each texture (e.g. color). These are combined in a Graphics3dGroup.
    • Grouping triangles by texture makes some sense because often there will be many triangles with the same texture.
    • plot3d_adaptive and GradualTextureTransform use the method IndexFaceSet.partition to partition triangles according to the values of a scalar function. In both cases this is done after a single IndexFaceSet has been constructed.
  • The color_data approach (setting attributes of individual faces as they're constructed) is faster than the GradualTextureTransform approach by nearly a factor of 2 (or 1/2; see timings below).
    • This makes sense, given that GradualTextureTransform has to construct the triangulation first and then partition it.
    • The color_data approach might be more memory-intensive, but I don't know how to measure this.

It could be possible to combine these two approaches:

  • Partition triangles according to texture and hence take advantage of the more robust support for textures in the graphics classes.
  • Construct this partition during the triangulation process, rather than after the fact.

Before I start working on this, please let me know if you have suggestions or concerns about this idea.

Thanks!

comment:78 Changed 4 years ago by niles

Oops, forgot the timing stuff.

First, general setup:

# setup
var('r v')
cmsel = [colormaps['autumn'](i) for i in sxrange(0,1,0.05)]
f(r,v) = 0.2*(r**2 + v**2) + cos(2*r)*sin(2*v)

from sage.plot.plot3d.texture_map import ColormapTransform
CMT = ColormapTransform(colormaps.autumn, 'height_z')

clebsch(x,y,z) = 81*(x^3+y^3+z^3)-189*(x^2*y+x^2*z+y^2*x+y^2*z+z^2*x+z^2*y) +54*x*y*z+126*(x*y+x*z+y*z)-9*(x^2+y^2+z^2)-9*(x+y+z)+1
def R(x,y,z): return z

Now compare no colormap, colormap with plot3d_adaptive, the color_data approach, and the GradualTextureTransform approach:

sage: # base
sage: %time q = plot3d(f,(r,-2,2), (v,-2,2))
CPU times: user 1.91 ms, sys: 75 µs, total: 1.98 ms
Wall time: 1.93 ms

sage: # with adaptive
sage: %time p = plot3d(f,(r,-2,2), (v,-2,2), adaptive=True, color=cmsel)
CPU times: user 29.2 ms, sys: 6.52 ms, total: 35.7 ms
Wall time: 31 ms

sage: # with color_data
sage: % time s = parametric_plot3d((r,v,f),(r,-2,2), (v,-2,2), color_data=(cf,colormaps.PiYG))
CPU times: user 1.89 ms, sys: 74 µs, total: 1.96 ms
Wall time: 1.92 ms

sage: # with GTT
sage: %time t = CMT.apply(parametric_plot3d((r,v,f),(r,-2,2), (v,-2,2)))
CPU times: user 4.55 ms, sys: 911 µs, total: 5.46 ms
Wall time: 4.84 ms

sage: %time t = CMT.apply(s)
CPU times: user 2.29 ms, sys: 724 µs, total: 3.01 ms
Wall time: 2.67 ms

An example that takes longer to plot, to see how adding colormap affects the time:

sage: %time k = implicit_plot3d(clebsch, (x,-2,2), (y,-2,2), (z,-2,2))
CPU times: user 6.71 ms, sys: 292 µs, total: 7 ms
Wall time: 6.88 ms

sage: %time m = implicit_plot3d(clebsch, (x,-2,2), (y,-2,2), (z,-2,2), color_data=(R,cm))
CPU times: user 7.89 ms, sys: 371 µs, total: 8.26 ms
Wall time: 7.98 ms

sage: %time j = CMT.apply(k)
CPU times: user 12.5 ms, sys: 2.86 ms, total: 15.3 ms
Wall time: 13.2 ms

comment:79 in reply to: ↑ 77 Changed 4 years ago by niles

Replying to niles:

It could be possible to combine these two approaches:

  • Partition triangles according to texture and hence take advantage of the more robust support for textures in the graphics classes.
  • Construct this partition during the triangulation process, rather than after the fact.

Before I start working on this, please let me know if you have suggestions or concerns about this idea.

Well, I've been thinking about this approach, and the fact that it would produce a different IndexFaceSet for every texture value. If only one texture parameter were to vary (e.g. color), then maybe the number of different objects would be manageable (around 20 different colors looks ok and is fast). But varying two or more attributes quickly puts the number of objects into the hundreds, and that's too many (too slow). Moreover, partitioning in this way looses the global structure of the object (this is used, e.g. in the triangulation code for ParametricSurface to see whether the surface is enclosed or not).

I also think it's worth noting the way tachyon handles textures, since presumably its author is more experienced than we are with these kinds of issues. Tachyon has a list of textures, and each object has a pointer to the texture which should be applied to it. I believe other rendering software works this way too (see Graphics3d.texture_set and Graphics3dGroup.texture_set). This is similar to the way IndexFaceSet handles faces (storing pointers to vertices, rather than the vertices themselves).

So maybe the ideal way forward would be to have each face of an IndexFaceSet have a pointer to a texture with the relevant color, opacity, etc. But it would be wasteful to do this by adding a new texture for every single face.

Anyway, now I've spent a nontrivial number of hours thinking about this, and I don't see an easy way forward. I'm just generally unhappy with the way textures are handled in the 3d plotting code, and that's not an issue for this ticket.

comment:80 Changed 4 years ago by niles

  • Branch changed from u/chapoton/12212 to u/niles/12212
  • Commit 41fe504cbf6dc5244e22bfd6dd4b374fd7d1eb28 deleted

Ok, I've made some very minimal changes so that the color keyword is used instead of color_data. I think my complaints are really related to larger issues in the plotting code, and I should stop getting in the way of this ticket.

I also removed the stuff in texture_map.py, namely GradualTextureTransform and related classes. I don't think it offers substantially different functionality from what's available in the rest of this patch (although it is slightly different, of course). I just couldn't find a strong justification for keeping it even though I'd like to. Please correct me if I'm wrong!

My changes are on the new branch u/niles/ticket/12212.

Last edited 4 years ago by niles (previous) (diff)

comment:81 Changed 4 years ago by niles

  • Branch changed from u/niles/12212 to u/niles/ticket/12212
  • Commit set to 26e78fa0dec9dd6d20fb7e2b849cda0d44049709

Last 10 new commits:

1f7fc20trac #12212 trying to enhance doctest coverage
92d9e5bMerge branch 'u/chapoton/12212' of ssh://trac.sagemath.org:22/sage into 12212
0ae10f5trac #12212 keeping the color keyword in json method
e28acd5Merge branch with 6.4.beta3
41fe504Merge with 6.4.beta4
ebb4fd5Merge branch 'develop' into u/chapoton/12212
765a8c4Minor edit in texture.py and some fixes for GradualTextureTransform.
91a1e45remove color_data keyword and overload color instead
6d891c1various minor fixes
26e78faremove texture_map.py (GradualTextureTransform etc.)

comment:82 Changed 4 years ago by git

  • Commit changed from 26e78fa0dec9dd6d20fb7e2b849cda0d44049709 to 1f5024d9dae78774a717218c791ef17111ba656c

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

1f5024dremove texture_map from reference manual

comment:83 Changed 4 years ago by kcrisman

Tachyon has a list of textures, and each object has a pointer to the texture which should be applied to it.

Wow, that is a lot smarter. I don't know if we can do that in pure Python, though.

I also removed the stuff in texture_map.py

I think we can keep that file in but remove it from the manual... I mean, as I see it (if I recall correctly), it does allow things to be applied after the fact, which could actually be useful (rather than starting a new plot from scratch).

chapoton, what do you think about Niles' ideas? I will be happy to check out the upstream sagenb ticket as well once I know that this solution is agreeable to both of you.

comment:84 follow-up: Changed 4 years ago by chapoton

  • Branch changed from u/niles/ticket/12212 to u/chapoton/12212
  • Commit changed from 1f5024d9dae78774a717218c791ef17111ba656c to 41fe504cbf6dc5244e22bfd6dd4b374fd7d1eb28

I have corrected one failing doctest related to Mobius band.

I am happy with the solution of using color instead of color_data.

I do not have a strong opinion about keeping or not the texture_map file. Maybe I would prefer to keep it, as it is the only way to get a transparency effect so far. But this should not stop the progress here. So either way is ok for me.

I think that we still do not ship a recent enough version of jmol that could accept the color information. Is this true ?

It would be good to have #7744 at our disposal in order to be able to save x3d files properly.

comment:85 Changed 4 years ago by git

  • Commit changed from 41fe504cbf6dc5244e22bfd6dd4b374fd7d1eb28 to e685cbc3c8c86426e8715bf1104054621b8777d0

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

ebb4fd5Merge branch 'develop' into u/chapoton/12212
765a8c4Minor edit in texture.py and some fixes for GradualTextureTransform.
91a1e45remove color_data keyword and overload color instead
6d891c1various minor fixes
26e78faremove texture_map.py (GradualTextureTransform etc.)
1f5024dremove texture_map from reference manual
e685cbctrac #12212 correct one failing doctest

comment:86 in reply to: ↑ 84 ; follow-ups: Changed 4 years ago by kcrisman

I do not have a strong opinion about keeping or not the texture_map file. Maybe I would prefer to keep it, as it is the only way to get a transparency effect so far. But this should not stop the progress here. So either way is ok for me.

Ah, then I suppose we should keep it.

I think that we still do not ship a recent enough version of jmol that could accept the color information. Is this true ?

I do not know, but I do know that you may want to check whether https://github.com/sagemath/sagenb/pull/212 is still the correct way to solve the canvas3d issue for these colormaps.

comment:87 in reply to: ↑ 86 ; follow-up: Changed 4 years ago by kcrisman

I think that we still do not ship a recent enough version of jmol that could accept the color information. Is this true ?

I do not know,

(Because your communication was private with jmol and I could never figure out exactly what commit or version of jmol that ended up in; if you have that info it shouldn't be too bad to do an upgrade of jmol for this ticket.)

comment:88 in reply to: ↑ 86 Changed 4 years ago by niles

Replying to kcrisman:

I do not have a strong opinion about keeping or not the texture_map file. Maybe I would prefer to keep it, as it is the only way to get a transparency effect so far. But this should not stop the progress here. So either way is ok for me.

Ah, then I suppose we should keep it.

Do you really think it's ready to be part of the Sage library? Joris said he wasn't happy with the class layout, and it hasn't changed substantially.

comment:89 in reply to: ↑ 87 Changed 4 years ago by niles

Replying to kcrisman:

I could never figure out exactly what commit or version of jmol that ended up in; if you have that info it shouldn't be too bad to do an upgrade of jmol for this ticket.

It was claimed that the functionality was in version 14.2.5 -- I don't know how to upgrade jmol though.

comment:90 Changed 4 years ago by chapoton

I have got two different info versions:

Later this week it will appear in Jmol 14.2.5. (10th of august)

Released as https://sourceforge.net/projects/jmol/files/Jmol-beta/Jmol%2014.3/Jmol%2014.3.6/

(the 18th of august)

I am not sure I understand their numbering system..

But this was probably just after the jmol we ship now (jmol-14.2.4_2014.08.03.tar.bz2)

comment:91 follow-up: Changed 4 years ago by kcrisman

In principle upgrading jmol in this minor version should be as simple as dumping a new tarball in the results of #17020. Assuming there aren't any other changes affecting us between those. It would be useful to know in which upstream commit that was fixed...

comment:92 in reply to: ↑ 91 ; follow-up: Changed 4 years ago by niles

Replying to kcrisman:

In principle upgrading jmol in this minor version should be as simple as dumping a new tarball in the results of #17020.

I'm afraid I don't understand . . . do we need to make a new ticket for upgrading jmol?

comment:93 in reply to: ↑ 92 ; follow-up: Changed 4 years ago by kcrisman

In principle upgrading jmol in this minor version should be as simple as dumping a new tarball in the results of #17020.

I'm afraid I don't understand . . . do we need to make a new ticket for upgrading jmol?

Not necessarily, it could be done here, especially if no other changes are needed. I mention #17020 because there was a slightly different naming convention or something. Anyway, you should be able to drop the new source tarball into upstream and then fix the checksums (http://www.sagemath.org/doc/developer/packaging.html#checksums) and do sage -f jmol to see what happens. Hopefully nothing bad!

comment:94 Changed 4 years ago by chapoton

Note that the jmol coloring must also be activated in sage by changing the lines

+        # put this into play when jmol will accept the colored pmesh
+        if True:   # self.global_texture:

inside index_face_set.pyx

comment:95 Changed 4 years ago by git

  • Commit changed from e685cbc3c8c86426e8715bf1104054621b8777d0 to 46de317e9c4f9c2a7a47e87cabcc9849338f0737

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

46de317trac #12212 activation of jmol coloring

comment:96 in reply to: ↑ 93 Changed 4 years ago by niles

  • Branch changed from u/chapoton/12212 to u/niles/12212
  • Commit 46de317e9c4f9c2a7a47e87cabcc9849338f0737 deleted

Replying to kcrisman:

Anyway, you should be able to drop the new source tarball into upstream and then fix the checksums (http://www.sagemath.org/doc/developer/packaging.html#checksums) and do sage -f jmol to see what happens. Hopefully nothing bad!

I upgraded jmol successfully, but no colormaps. However I did notice a serious problem with the try/except checks for the color keyword: TypeError is the error when something doesn't have len(). I've fixed that now.


p.s. If someone else wants to try upgrading jmol, there's a little more to it than what Karl said, but not much. You have to make the tarball in the right way, and this is described in build/pkgs/jmol/SPKGS.txt. You also have to update the version number in build/pkgs/jmol/package-version.txt.

comment:97 Changed 4 years ago by niles

  • Branch changed from u/niles/12212 to u/niles/ticket/12212
  • Commit set to dabfdc4f85925fffe305c1f88d212031243ee8ef

Last 10 new commits:

91a1e45remove color_data keyword and overload color instead
6d891c1various minor fixes
26e78faremove texture_map.py (GradualTextureTransform etc.)
1f5024dremove texture_map from reference manual
e685cbctrac #12212 correct one failing doctest
46de317trac #12212 activation of jmol coloring
f7321b7Update to jmol 14.3.6_2014.08.17b
591921aMerge branch 'develop' into ticket/12212
8313589Revert "Update to jmol 14.3.6_2014.08.17b"
dabfdc4fix try/except for color keyword

comment:98 follow-up: Changed 4 years ago by chapoton

Could you please provide a link to the tar ball you made ?

comment:99 in reply to: ↑ 98 ; follow-up: Changed 4 years ago by niles

Replying to chapoton:

Could you please provide a link to the tar ball you made ?

ok, this should work for a while

https://www.dropbox.com/s/lh4p5vx3rryzw2i/jmol-14.3.6_2014.08.17b.tar.bz2?dl=0

comment:100 in reply to: ↑ 99 Changed 4 years ago by gutow

Replying to niles:

https://www.dropbox.com/s/lh4p5vx3rryzw2i/jmol-14.3.6_2014.08.17b.tar.bz2?dl=0

Just a reminder that the odd decimal releases are the development branch not the stable branch. This is OK for testing, but we will not roll another Jmol spkg that includes the changes in this until the 14.4 release. If there are major bug fixes I may make another 14.2 spkg.

comment:101 follow-up: Changed 4 years ago by kcrisman

Umm... would it be possible to cherry-pick the (presumably small?) commit that is needed for this functionality and add it as a patch in spkg-install?

comment:102 in reply to: ↑ 101 Changed 4 years ago by niles

Replying to kcrisman:

Umm... would it be possible to cherry-pick the (presumably small?) commit that is needed for this functionality and add it as a patch in spkg-install?

In principle, sure. But we still don't know exactly which commit that was, and I'm not particularly interested in learning how to do spkg patches at the moment. So instead I say we forget about jmol on this ticket. As I understand it, this will just automatically start to work when/if jmol is upgraded -- is that right? In the meantime, do we need to revert 46de317e9?

comment:103 Changed 4 years ago by chapoton

Yes I think this commit has to be reverted, though I have not tested to see what happens if we do not revert it.

comment:104 Changed 4 years ago by kcrisman

In principle, sure. But we still don't know exactly which commit that was,

I was asking Jonathan and chapoton, of course, as that was my point too. I'm sure chapoton or myself would be happy to turn that into a patch, we both have some experience with that. But if it's not identifiable, then that sort of misses the point.

For the record, apparently 14.2.9 is the latest stable branch version. Maybe someone can search here for whether the feature we want is there. Say,

new feature: pmesh files can contain triangle colors

which should in fact already be in the version Niles made! Maybe the interface is not as obvious? At any rate, that should definitely be in 14.2.9, according to this. It must be around commit r19940 (see the browser) but I can't find it.

comment:105 Changed 4 years ago by gutow

If it is in the 14.2 release series we should just make a new spkg part of this. The 14.2 series is stable enough for production. It is very easy to make a new Jmol spkg.

comment:106 Changed 4 years ago by kcrisman

It would be useful to make a 14.2.9 spkg and then see whether this change (which certainly should be in it, though I couldn't find the commit for it, as it is in the release notes which seem to be auto-generated) actually does anything. Then no one has to revert any commits. But I think maybe there is more to allowing the triangles to be colored than this (on the Sage side), maybe?

comment:107 follow-up: Changed 4 years ago by chapoton

I have made an spkg here :

https://chapoton.perso.math.cnrs.fr/public/jmol-14.2.9_2014.11.23.tar.bz2

I hope I have followed correctly the instruction of the jmol SPKG.txt

I have not changed the checksums.

comment:109 Changed 4 years ago by chapoton

a login ? it should not, let me investigate

Ok, the link is http://chapoton.perso.math.cnrs.fr/jmol-14.2.9_2014.11.23.tar.bz2

Here is a new branch with checksums of jmol 14.2.9

not yet tested

Last edited 4 years ago by chapoton (previous) (diff)

comment:110 Changed 4 years ago by kcrisman

I can confirm that with this updated spkg that it works for Tachyon but only gives a blank jmol (this is in the command line). So something must be wrong about how we are passing that information to jmol, I guess. The interesting thing is that comment:51 indicates that the change must have worked at some point...

(By the way, I don't believe the branch here was updated to 14.2.9; I did that on my own, but presumably the outcome is the same.)

comment:111 Changed 4 years ago by chapoton

  • Branch changed from u/niles/ticket/12212 to u/chapoton/12212
  • Commit changed from dabfdc4f85925fffe305c1f88d212031243ee8ef to d6d60edfcd4524d882e14b5cd4f97b26a79e28d6

New commits:

d6d60edtrac #12212 updating jmol package to 14.2.9

comment:112 follow-up: Changed 4 years ago by chapoton

Ok, bad news and good news..

Good news, the canvas plotting works and the tachyon plotting works

Bad news, the jmol plotting does not recognize the colors (but it works and stays blue)

I have pulled the latest sagenb from git, and it works very badly : I cannot evaluate cells..

I must say that picture of comment:51 was send to me by the man from jmol. I have never myself seen jmol running with this kind of color mesh.

comment:113 in reply to: ↑ 112 Changed 4 years ago by kcrisman

Good news, the canvas plotting works and the tachyon plotting works

Yay!

Bad news, the jmol plotting does not recognize the colors (but it works and stays blue)

Huh, what command did you use precisely?

I have pulled the latest sagenb from git, and it works very badly : I cannot evaluate cells..

Probably you need to delete your cache; you may still not have the latest jQuery, that is usually what caused this various times for people using the develop stuff.

I must say that picture of comment:51 was send to me by the man from jmol. I have never myself seen jmol running with this kind of color mesh.

Perhaps another well-placed email to ask if there is anything else we need to know would not be amiss...

comment:114 Changed 4 years ago by kcrisman

It also occurs to me that if jmol ended up working, we would need to amend this branch anyway, because there are several references to how jmol does not work! If we can resolve it within a week I say let's do so, otherwise I think it's better to have this in with the various warnings against using jmol BUT working hard at resolving that, since most people would be using that.

comment:115 Changed 4 years ago by kcrisman

Okay, jmol does indeed 'work' (though without the colormap) in command line and notebook, though the Tachyon pic is the static piece in the notebook, I think.

sage: from sage.plot.plot3d.parametric_surface import ParametricSurface
sage: cm = colormaps.autumn
sage: def c(x,y): return sin(x*y)**2
sage: def g(x,y): return x, y, x**2 + y**2
sage: P1 = ParametricSurface(g, (srange(-10,10,0.1), srange(-5,5.0,0.1)),color=(c,cm))
sage: P1.show() 

However,

sage: sage: var('x,y,z')
(x, y, z)
sage: t = (sin(2*y+3*z)**2).function(x,y,z)
sage: cm = colormaps.gist_rainbow
sage: sage: implicit_plot3d(x^2+y^2+z^2==4, (x, -3, 3), (y, -3,3), (z, -3,3), color=(t,cm))

doesn't. Given that this is supposed to work with Tachyon, it should presumably at least fail gracefully for Jmol. Is this maybe a difference between parametric and implicit plotting?

Changed 4 years ago by chapoton

a jmol colored filed

comment:116 follow-up: Changed 4 years ago by chapoton

I have attached a pmesh file (in the format produced by sage) that is supposed to work. Trying using the console in jmol:

pmesh "/home/chapoton/nice_shell-obj_1.pmesh" fullylit

this seems to load, but does not show anything

comment:117 Changed 4 years ago by chapoton

The script file we send to jmol contains a suspect line

color pmesh  [102,102,255]

which probably turns everything blue..

comment:118 Changed 4 years ago by gutow

I got essentially what is shown in comment 51 (it may be inside out) from nice_shell-obj_1.pmesh. I just did a simple pmesh "path_to_file" command in the Jmol 14.2.7 console to get this. I verified that it also works with JSmol-14.2.7 inside of sage by uploading the pmesh as a data file and then issuing the command pmesh "/home/<user_name>/<worksheet_number>/data/nice_shell-obj_1.pmesh" in the console of a running JSmol. I also had to issue a pmesh fullylit command after the load to get the outside of the shell bright.

Thus this should work right if the commands generated to load the pmesh are correct. The issue is with the .jmol script file being generated.

Last edited 4 years ago by gutow (previous) (diff)

comment:119 follow-up: Changed 4 years ago by kcrisman

Now you are all beyond my own expertise, but it sounds like things are on the right track, so I'll test this as an "independent" observer as soon as this is solved.

By the way, any ideas on why my implicit plot in comment:115 doesn't work (in jmol, I mean)?

comment:120 in reply to: ↑ 116 Changed 4 years ago by gutow

I think the problem is that your path is wrong. JSmol because it is inside the browser is limited to pmesh commands that request things through the server. See my comment 118. There is a load from local file feature in the popup menu, but that cannot identify the type of the text pmesh file. Replying to chapoton:

I have attached a pmesh file (in the format produced by sage) that is supposed to work. Trying using the console in jmol:

pmesh "/home/chapoton/nice_shell-obj_1.pmesh" fullylit

this seems to load, but does not show anything

comment:121 in reply to: ↑ 119 ; follow-up: Changed 4 years ago by gutow

Replying to kcrisman:

By the way, any ideas on why my implicit plot in comment:115 doesn't work (in jmol, I mean)?

Is it related to ^ vs. **? In sage-6.4 I keep getting errors when I use the first form for exponentiation.

comment:122 in reply to: ↑ 121 Changed 4 years ago by kcrisman

Is it related to ^ vs. **? In sage-6.4 I keep getting errors when I use the first form for exponentiation.

??? That would be a huge bug (and one that should in principle be caught hundreds of times in the test suite). In any case, that example works with Tachyon and doesn't raise an error in jmol (just gives a blank white jmol), so that is not the issue.

comment:123 Changed 4 years ago by git

  • Commit changed from d6d60edfcd4524d882e14b5cd4f97b26a79e28d6 to 3ac721d126fee2e7704b6db62e5152afbf49a19c

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

3ac721dtrac #12212 let jmol show the colors at last

comment:124 Changed 4 years ago by chapoton

Progress: I have been able to see Jmol showing the color for a parametric plot.

Implicit plot still not working (sigh).

comment:125 Changed 4 years ago by kcrisman

I can confirm this works! Almost there...

comment:126 Changed 4 years ago by chapoton

Does this one works for you in jmol ? It works for me !

sage: t = (1-sin(2*x*y+3*z)**2).function(x,y,z)
sage: cm = colormaps.autumn
sage: G = ImplicitSurface(x^2 + y^2 + z^2, (x,-2, 2), (y,-2, 2), (z,-2, 2), contour=4, color=(t,cm))
sage: G.show()

But fails with cm = colormaps.terrain

And your example of comment:115 works with colormaps.YlOrBr

So this could be a problem with color ranges.. Maybe for colors not fitting in int ?

But the "bad" colormaps work for parametric surfaces..

Last edited 4 years ago by chapoton (previous) (diff)

comment:127 follow-up: Changed 4 years ago by kcrisman

Nice. Weird that some colormaps work and others don't. I wonder if some categories in the list of mpl colormaps work and others don't. Anyway, that is very good news.

comment:128 Changed 4 years ago by kcrisman

Maybe for colors not fitting in int ?

Sure, or some other issue along those lines (float conversion, who knows what).

comment:129 Changed 4 years ago by kcrisman

Or maybe it's your crazy t functions; after all, the colormaps can't color something that is too big/small, right? What if one uses a different t...

comment:130 in reply to: ↑ 127 Changed 4 years ago by kcrisman

Nice. Weird that some colormaps work and others don't. I wonder if some categories in the list of mpl colormaps work and others don't. Anyway, that is very good news.

I bet it's just the 'Miscellaneous' ones. The 'Sequential' ones are fine.

comment:131 follow-up: Changed 4 years ago by kcrisman

In fact, my semi-random testing indicates that even among those, only a few don't work. The other categories seem fine. Rather than fix this here (which is likely some unusual function of t, implicit surface versus parametric, and who knows what else), I would instead do the following.

  • Amend the doc to remove suggestions jmol doesn't work
  • Add extra examples to show various viewers working (possibly not tested if they rely on the notebook like canvas3d)
  • Add an example warning of a colormap that does not work with jmol for whatever reason.

I'll double-check all this works in the notebook next.

comment:132 Changed 4 years ago by chapoton

Compare

    cdef apply_color_func(self, color_c *pt, fn, cm, VertexInfo v):
        t = fn(v.eval_pt.x, v.eval_pt.y, v.eval_pt.z)
        pt[0].r, pt[0].g, pt[0].b, _ = cm(t)

and

                if self.color_function is not None:
                    face.color.r, face.color.g, face.color.b, _ = self.colormap(self.color_function(urange[i], vrange[j]))


Maybe the different behaviour comes from here..

comment:133 in reply to: ↑ 131 Changed 4 years ago by kcrisman

I'll double-check all this works in the notebook next.

Interestingly, even the ones that give a 'blank' jmol from the command line give a more-or-less black object of the right shape in jsmol. That is very interesting - one can test it by switching in the notebook from load 3d live or not.

Though also in my testing some that worked in command line turned black in notebook... strange.

comment:134 Changed 4 years ago by kcrisman

Maybe the different behaviour comes from here..

This confirms my thought that for some choices of colormaps and color evaluation functions the colormaps just don't work right with jmol (too large of values or too close to zero or whatever). For instance,

var('x,y,z')
t = (x+y+z).function(x,y,z)
cm = colormaps.terrain
implicit_plot3d(x^2+y^2+z^2==4, (x, -3, 3), (y, -3,3), (z, -3,3), color=(t,cm))

works fine in jsmol and jmol, while with the sin functions you were using it does not. I think the "blank" ones are ones where the values are too close to zero, or something like that. Switch to

cm = colormaps.flag

and you will have a big surprise - Tachyon, jmol, and jsmol are all different!

comment:135 follow-up: Changed 4 years ago by kcrisman

  • Milestone changed from sage-6.4 to sage-6.5
  • Reviewers changed from Frédéric Chapoton, Niles Johnson to Frédéric Chapoton, Niles Johnson, Karl-Dieter Crisman, Jonathan Gutow
  • Work issues set to update documentation and examples

Nevertheless, as long as we warn against certain colormaps and so forth (see comment:131) I think this should be added.

Niles, do you have any particular comments beyond this? Thanks for helping us figure out where to go from here.

comment:136 in reply to: ↑ 135 Changed 4 years ago by niles

Replying to kcrisman:

Nevertheless, as long as we warn against certain colormaps and so forth (see comment:131) I think this should be added.

Niles, do you have any particular comments beyond this? Thanks for helping us figure out where to go from here.

Glad to see the progress! My window of time to work on this has closed. In general I am nervous about putting something in that sometimes works and sometimes doesn't for reasons we don't understand. If not done very carefully, this could make the plotting code look really unprofessional. Hopefully you guys will sort out where the bugs are.

comment:137 Changed 4 years ago by chapoton

Little progress.

The problem appears using only the pmesh files and not the script. So the problem is in the pmesh files.

1) it seems that Jmol does not like the color 0 at all. Replacing 0 by 1 enhances things a little bit. This is a symptomatic cure of only part of the problem.

2) but the example of comment:115 still does not work

I suspect that there could be some problem inside Jmol about the treatment of colored pmesh. It may be difficult to investigate without close contact with the Jmol team.

Here is a new simple testing case

var('x,y,z')
t = z.function(x,y,z)
cm = colormaps.flag
P = implicit_plot3d(z-x*x-y*y,(x,-2,2),(y,-2,2),(z,-1,4),color=(t,cm))

Was not working with Jmol Now works correctly with Jmol Produces spectacular unexpected special effects with Jsmol

comment:138 Changed 4 years ago by git

  • Commit changed from 3ac721d126fee2e7704b6db62e5152afbf49a19c to e88ee7823deecf234111cd60af98d1cc75f175b0

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

e88ee78trac #12212 various things

comment:139 Changed 4 years ago by chapoton

I suspect something bad happening when one of the color numbers coincide with a vertex number. Definitely not something inside sage..

comment:140 Changed 4 years ago by kcrisman

Thanks for the progress!

  • There should probably be one (working) example each without a viewer option.
  • Do we need the colormap warning info in the parametric plot, or just implicit? Maybe we just have gotten lucky with the parametric ones so far because they didn't have a color close to zero.
  • Probably the warning thing should follow the warning block syntax here.

comment:141 Changed 4 years ago by git

  • Commit changed from e88ee7823deecf234111cd60af98d1cc75f175b0 to 669d270b343da222aa68fcf40147c7cb6d0ad5c8

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

669d270trac #12212 doc changes, some examples with show

comment:142 Changed 4 years ago by kcrisman

My window of time to work on this has closed.

Yes, mine really should have as well... thanks for the help!

In general I am nervous about putting something in that sometimes works and sometimes doesn't for reasons we don't understand. If not done very carefully, this could make the plotting code look really unprofessional.

Well, I guess. My feeling is that the warning and the fact that not too many colormaps are bad is okay. Though I think it does need a bit more testing in the jsmol, which seems to be the most annoying. If I'm lucky I can even make a list of "bad" colormaps.

Hopefully you guys will sort out where the bugs are.

It's pretty clear they are in jmol somewhere, since the other viewers work very nicely.

comment:143 Changed 4 years ago by kcrisman

Sorry, the warning needs to be in parametric_surface.pyx too, and changed since jmol is sort of working.

In that file, you say

Note that the coloring function should have values between 0 and 1. This value is passed to the chosen colormap.

Is that maybe the problem we are having? What happens with values outside that - is it done 'modulo 1' or something else?

In implicit_plot I would put the examples of this in the example section, maybe after the smooth=True example.

In parametric plot 3d the examples need to be dealt with anyway, and the indentation is not right when you view it.

    EXAMPLES: We demonstrate each of the four ways to call this
    function.


    #. A space curve defined by three functions of 1 variable:

       ::

           sage: parametric_plot3d( (sin, cos, lambda u: u/10), (0, 20))
           Graphics3d Object

       Note above the lambda function, which creates a callable Python
       function that sends `u` to `u/10`.

    #. Next we draw the same plot as above, but using symbolic
       functions:

       ::

           sage: u = var('u')
           sage: parametric_plot3d( (sin(u), cos(u), u/10), (u, 0, 20))
           Graphics3d Object

    #. We draw a parametric surface using 3 Python functions (defined
       using lambda):

       ::

           sage: f = (lambda u,v: cos(u), lambda u,v: sin(u)+cos(v), lambda u,v: sin(v))
           sage: parametric_plot3d(f, (0, 2*pi), (-pi, pi))
           Graphics3d Object

up to here is fine, but

    #. The surface, but with a mesh:

       ::

           sage: u, v = var('u,v')
           sage: parametric_plot3d((cos(u), sin(u) + cos(v), sin(v)), (u, 0, 2*pi), (v, -pi, pi), mesh=True)
           Graphics3d Object

should just be an example later. This one is the true "fourth way to call the function".

    #. The same surface, but where the defining functions are
       symbolic:

       ::

           sage: u, v = var('u,v')
           sage: parametric_plot3d((cos(u), sin(u) + cos(v), sin(v)), (u, 0, 2*pi), (v, -pi, pi))
           Graphics3d Object

Everything after that is indented too far, up until

    We call the space curve function but with polynomials instead of
    symbolic variables.

including all the added examples.

comment:144 Changed 4 years ago by git

  • Commit changed from 669d270b343da222aa68fcf40147c7cb6d0ad5c8 to be9d2a587dc3d7bfcb9c9e19689e8fa2feec104b

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

be9d2a5trac #12212 twists in the doc

comment:145 Changed 4 years ago by chapoton

I have done the following:

  • take a failing example of pmesh file
  • replace all the colors by a fixed one using a text editor

and then it worked.

I think that our color numbers are valid (between 0 and 2**24)

So it seems that the import process of jmol is broken somewhere. I have contacted upstream on the subject.

comment:146 Changed 4 years ago by kcrisman

Try this.

var('x,y,z')
t = sin(z).function(x,y,z)
@interact
def _(cm = [colormaps[c] for c in colormaps],v = ['jmol','tachyon']):
    show(implicit_plot3d(z-x*x-y*y,(x,-2,2),(y,-2,2),(z,-1,4),color=(t,cm),viewer=v))

(Could be slow.)

I don't know if I like having a switch to 1, though, as most of the colormaps seem to mostly care about 0 to 1 or -1 to 1 as color values. (Switch the function about to z instead of sin(z) to see this.) That could be a somewhat bizarre change. What if you switched it from 0 to 0.01 or something like that?

For instance,

var('x,y,z')
t = SR(0).function(x,y,z)
implicit_plot3d(z-x*x-y*y,(x,-2,2),(y,-2,2),(z,-1,4),color=(t,colormaps.terrain)) # or flag or whatever

this seems to work fine. Changing to SR(1) gives quite a different color! So I would recommend not putting in that bit where you check for the 0 color - especially since the other viewers work fine! No need to mess them up.

Anyway, as far as I'm concerned the fault here clearly lies with jmol or jsmol, and is not so horrible as to keep us from putting this in.

comment:147 Changed 4 years ago by chapoton

Thanks for the interactive. Here is a slightly modified version, with canvas3D and colormap names

var('x,y,z')
t = sin(z).function(x,y,z)
@interact
def _(cm=[colormaps[c].name for c in colormaps], v=['jmol', 'tachyon', 'canvas3d']):
     show(implicit_plot3d(z-x*x-y*y,(x,-2,2),(y,-2,2),(z,-1,4),color=(t,colormaps[cm]),viewer=v))

One can note the canvas3d does not have the same viewpoint. One can also not that the colors do not look quite the same between the viewers. Maybe a difference in global lighting ?

Concerning setting the color 0 to 1 : it happens inside a jmol specific function, and after the color has been turned into an integer between 0 and 2**24. So changing 0 to 1 here does not affect at all the other viewers, and should only change total black into almost total black, so should not have any visible effect on jmol output (except making it work slightly more often).

I am also not quite satisfied with this buggy behaviour, but something is better than nothing. Nevertheless it could still be that the problem is in our dialog with jmol and not in jmol itself : are we coding the colors correctly ? I thought so, but maybe this needs to be checked. I have tried to navigate the jmol code, but did not manage to find something useful about color encoding.

comment:148 Changed 4 years ago by git

  • Commit changed from be9d2a587dc3d7bfcb9c9e19689e8fa2feec104b to ccef191ffe9032f39ae95ffcf101045b924a5f16

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

ccef191trac #12212 some doc details

comment:149 follow-up: Changed 4 years ago by chapoton

I have got one answer from upstream. They are working on correcting jmol, which has problems when there are two many colors. If they succeed, it would mean that hopefully our issues will be solved in a new release of jmol.

comment:150 Changed 4 years ago by git

  • Commit changed from ccef191ffe9032f39ae95ffcf101045b924a5f16 to ffe6d0f96b780bb4dc968f70b325635fad1d3388

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

ffe6d0ftrac #12212 more little doc and code details

comment:151 in reply to: ↑ 149 Changed 4 years ago by kcrisman

I have got one answer from upstream. They are working on correcting jmol, which has problems when there are two many colors. If they succeed, it would mean that hopefully our issues will be solved in a new release of jmol.

I would say that's worth waiting for.

comment:152 Changed 4 years ago by chapoton

A corrected jmol is available here

http://chapoton.perso.math.cnrs.fr/jmol-14.3.11_2015.01.14.tar.bz2

A branch to use that version of jmol is available under the name public/12212-experimental

It seems inded to solve our problems. This probably needs more testing. Upstream is waiting for our feedback. One can hope that this will get into a stable release.

comment:153 Changed 4 years ago by chapoton

  • Status changed from needs_work to needs_review

A stable jmol is available here

​​http://chapoton.perso.math.cnrs.fr/jmol-14.2.11_2015.01.20.tar.bz2

As far as I can tell, this works very well.

This seems to be ready to go ! I will now update the branch here.

comment:154 Changed 4 years ago by git

  • Commit changed from ffe6d0f96b780bb4dc968f70b325635fad1d3388 to f6251ae9d3c710503d13e6f8ab554afe359103c7

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

f6251aetrac #12212 now with a stable jmol release !

comment:155 Changed 4 years ago by chapoton

  • Work issues update documentation and examples deleted

comment:156 Changed 4 years ago by git

  • Commit changed from f6251ae9d3c710503d13e6f8ab554afe359103c7 to 36e4d8cae0037fc8442fbd20594a8e36c5b46196

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

36e4d8ctrac #12212 removal of warnings

comment:157 Changed 4 years ago by kcrisman

Awesome! I will try to deal with this tonight or tomorrow.

comment:158 Changed 3 years ago by kcrisman

Is this still needed?

+        # it seems that Jmol does not like the 0 color at all
+        if color == 0:
+            color = 1

comment:159 Changed 3 years ago by kcrisman

Anyway, it seems it doesn't matter for now. I am very happy with how this turned out and shall also merge the upstream canvas3d patch (which will be in #17645).

Assuming there is no further review required? Frédéric, did you positively review all Niles' changes? I think your stuff is mostly doc and the upgrade in jmol (plus actually fixing the pmesh).

comment:160 Changed 3 years ago by kcrisman

  • Dependencies set to #17645

comment:161 Changed 3 years ago by chapoton

As far as I am concerned, I am happy with the current state of the branch.

So positive review for me. If you agree, please set this status.

comment:162 Changed 3 years ago by jdemeyer

  • Priority changed from major to blocker

comment:163 Changed 3 years ago by kcrisman

  • Status changed from needs_review to positive_review

Interestingly, looking at the very initial example,

sage: cmsel = [colormaps['autumn'](i) for i in sxrange(0,1,0.05)]
sage: var('x,y,z')
sage: implicit_plot3d(x^2+y^2+z^2==4, (x, -3, 3), (y, -3,3), (z, -3,3), color=cmsel)
TypeError: 'NoneType' object is not iterable

still happens, since we need a tuple or something. But weirdly, this is the only way to get colormaps to work with plot3d itself! (As opposed to doing a "trivial" parametric plot.)

So... are we saying that we now have colormaps, after a Herculean effort, for implicit and parametric plots, but not for "regular" plots? There is nothing in http://sagemath.org/doc/reference/plot3d/sage/plot/plot3d/plot3d.html that would suggest we do.

Needless to say, I am not holding this up for that, but I have also to say that the complete lack of documentation (William's comment in Niles original question speaks to this) for the plot3d side definitely requires another ticket, and hopefully even the same syntax could work, despite what I now agree with Niles is definitely messed-up internals on such things.

See #17660.

comment:164 Changed 3 years ago by kcrisman

  • Authors changed from Joris Vankerschaver, Frédéric Chapoton to Joris Vankerschaver, Frédéric Chapoton, Niles Johnson

comment:165 Changed 3 years ago by fbissey

  • Description modified (diff)

When you have a new version of a package, please put it in the ticket description so that it is easy to find.

comment:166 Changed 3 years ago by kcrisman

Sorry, the discussion had been going so long here... but hopefully soon over!

comment:167 Changed 3 years ago by jdemeyer

  • Description modified (diff)

comment:168 Changed 3 years ago by vbraun

  • Branch changed from u/chapoton/12212 to 36e4d8cae0037fc8442fbd20594a8e36c5b46196
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.