sagemath / sage

Main repository of SageMath
https://www.sagemath.org
Other
1.42k stars 476 forks source link

Incorrect json representation for graphics #22253

Closed fchapoton closed 7 years ago

fchapoton commented 7 years ago

see https://groups.google.com/forum/#!topic/sage-devel/Wjo7NkmvCks

The problem lies in the file src/sage/plot/plot3d/index_face_set.pyx, where the json is generated. There are two problems with the generated json:

  1. The quote signs used in the file are ' and not " as per Json spec.
  2. Every dictionary key is unquoted, which is incorrect. Thus, a point

    {x:0,y:0,z:0}

    should really be

    {"x":0,"y":0,"z":0}

    as per Json spec.

CC: @novoselt @egourgoulhon

Component: graphics

Author: Frédéric Chapoton

Branch/Commit: fbd8b10

Reviewer: Paul Masson

Issue created by migration from https://trac.sagemath.org/ticket/22253

fchapoton commented 7 years ago

Commit: 9a7269f

fchapoton commented 7 years ago

Branch: u/chapoton/22253

fchapoton commented 7 years ago

Description changed:

--- 
+++ 
@@ -1 +1,15 @@
 see https://groups.google.com/forum/#!topic/sage-devel/Wjo7NkmvCks
+
+
+The problem lies in the file `src/sage/plot/plot3d/index_face_set.pyx`, where the json is generated. There are two problems with the generated json:
+
+1. The quote signs used in the file are `'` and not `"` as per [Json spec](http://www.ecma-international.org/publications/files/ECMA-ST/ECMA-404.pdf).
+2. Every dictionary key is unquoted, which is incorrect. Thus, a point 
+    
+        {x:0,y:0,z:0}
+        
+    should really be
+
+        {"x":0,"y":0,"z":0}
+        
+    as per Json spec.
fchapoton commented 7 years ago

Author: Frédéric Chapoton

fchapoton commented 7 years ago

New commits:

9a7269ftrac 22253 fixing json repr of index_face_set
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 7 years ago

Changed commit from 9a7269f to 5f7e950

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 7 years ago

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

5f7e950trac 22253 fix doctests
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 7 years ago

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

11e4447trac 22253 fix more doctests
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 7 years ago

Changed commit from 5f7e950 to 11e4447

fchapoton commented 7 years ago
comment:4

Seems ok with the canvas3d viewer (in sagenb):

sage: u,v = var('u,v')
sage: def cf(u,v): return sin(u+v/2)**2
sage: P = parametric_plot3d((cos(u), sin(u)+cos(v), sin(v)),
....: (u,0,2*pi), (v,-pi,pi), color=(cf,colormaps.PiYG), plot_points=[60,60])
sage: P.show(viewer='canvas3d')

Needs to be tested in other kinds of notebooks.

Ok also in jupyter notebook. Remains to check in cloud (not that easy).

53959995-fd20-47f5-85e6-5e769b863d1f commented 7 years ago
comment:5

All doctests pass in sage/plot/plot3d. Works with the new native Three.js viewer.

I would like to point out that this modification will bloat Three.js files without any change to the rendering, since JavaScript doesn't care whether array keys are quoted or not. Presumably that was part of the motivation for using formally incorrect JSON to begin with, so I didn't change it when writing the Three.js viewer.

53959995-fd20-47f5-85e6-5e769b863d1f commented 7 years ago

Reviewer: Paul Masson

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 7 years ago

Changed commit from 11e4447 to d04e1b5

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 7 years ago

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

e98721dMerge branch 'u/chapoton/22253' in 7.6.b1
d04e1b5trac 22253 one more detail
fchapoton commented 7 years ago
comment:7

one still needs to change

local/share/sage/ext/doctest/rich_output/example.canvas3d

but this is not under git control ?

fchapoton commented 7 years ago
comment:8

Attachment: example.canvas3d.gz

I have attached the modified example file. What now ?

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 7 years ago

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

061c579Merge branch 'u/chapoton/22253' in 7.6.b3
969685atrac 22253 fixing the canvas3d example file
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 7 years ago

Changed commit from d04e1b5 to 969685a

fchapoton commented 7 years ago
comment:10

This should be good now, but patchbots will not have a look. Please check and review.

53959995-fd20-47f5-85e6-5e769b863d1f commented 7 years ago
comment:11

Failing doctest from most recent alterations:

File "src/sage/repl/rich_output/output_graphics3d.py", line 159, in sage.repl.rich_output.output_graphics3d.OutputSceneCanvas3d.example
Failed example:
    rich_output.canvas3d
Expected:
    buffer containing 649 bytes
Got:
    buffer containing 829 bytes
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 7 years ago

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

111dfe2Merge branch 'u/chapoton/22253' in 7.6.b4
fbd8b10trac 22253 fixing doctest, plus remove trailing whitespaces
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 7 years ago

Changed commit from 969685a to fbd8b10

fchapoton commented 7 years ago
comment:13

done, should be good now.

vbraun commented 7 years ago

Changed branch from u/chapoton/22253 to fbd8b10