Opened 6 years ago

Closed 5 years ago

#22253 closed defect (fixed)

Incorrect json representation for graphics

Reported by: chapoton Owned by:
Priority: major Milestone: sage-7.6
Component: graphics Keywords:
Cc: novoselt, egourgoulhon Merged in:
Authors: Frédéric Chapoton Reviewers: Paul Masson
Report Upstream: N/A Work issues:
Branch: fbd8b10 (Commits, GitHub, GitLab) Commit: fbd8b10344636dc09211356be7daaf2f1ff25f38
Dependencies: Stopgaps:

Status badges

Description (last modified by chapoton)

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.

Attachments (1)

example.canvas3d (829 bytes) - added by chapoton 6 years ago.

Download all attachments as: .zip

Change History (16)

comment:1 Changed 6 years ago by chapoton

  • Authors set to Frédéric Chapoton
  • Branch set to u/chapoton/22253
  • Commit set to 9a7269f0aa771d5a755384d65c4eee55adb90b0d
  • Description modified (diff)
  • Status changed from new to needs_review

New commits:

9a7269ftrac 22253 fixing json repr of index_face_set

comment:2 Changed 6 years ago by git

  • Commit changed from 9a7269f0aa771d5a755384d65c4eee55adb90b0d to 5f7e950acf177fb89023445f6e331861fd8dcfef

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

5f7e950trac 22253 fix doctests

comment:3 Changed 6 years ago by git

  • Commit changed from 5f7e950acf177fb89023445f6e331861fd8dcfef to 11e4447438ab44199fc3b0d36e95416a8e4459c4

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

11e4447trac 22253 fix more doctests

comment:4 Changed 6 years ago by chapoton

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

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

comment:5 Changed 6 years ago by paulmasson

  • Cc novoselt egourgoulhon added
  • Reviewers set to Paul Masson

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.

comment:6 Changed 6 years ago by git

  • Commit changed from 11e4447438ab44199fc3b0d36e95416a8e4459c4 to d04e1b5ea463681f2f1288e600de3022381f96f6

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

comment:7 Changed 6 years ago by chapoton

one still needs to change

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

but this is not under git control ?

Changed 6 years ago by chapoton

comment:8 Changed 6 years ago by chapoton

I have attached the modified example file. What now ?

comment:9 Changed 6 years ago by git

  • Commit changed from d04e1b5ea463681f2f1288e600de3022381f96f6 to 969685a368ceabef622c888ace30f2131a666bb1

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

comment:10 Changed 6 years ago by chapoton

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

comment:11 Changed 6 years ago by paulmasson

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

comment:12 Changed 5 years ago by git

  • Commit changed from 969685a368ceabef622c888ace30f2131a666bb1 to fbd8b10344636dc09211356be7daaf2f1ff25f38

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

comment:13 Changed 5 years ago by chapoton

done, should be good now.

comment:14 Changed 5 years ago by paulmasson

  • Status changed from needs_review to positive_review

comment:15 Changed 5 years ago by vbraun

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