Opened 15 months ago

Closed 13 months ago

Last modified 13 months ago

#28949 closed defect (duplicate)

sage.plot.plot3d.shapes2.Line does not accept Sage numbers any more

Reported by: dimpase Owned by:
Priority: major Milestone: sage-duplicate/invalid/wontfix
Component: graphics Keywords: threejs
Cc: Merged in:
Authors: Reviewers: Paul Masson
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Status badges

Description (last modified by dimpase)

The following worked in 8.9, but is broken in Sage 9.0:

sage: from sage.plot.plot3d.shapes2 import Line
sage: X = Line([(-1, 0, 0), (0, 0, 0), (1, 0, 0)])
sage: X
/home/dimpase/sage/local/lib/python3.7/site-packages/sage/repl/rich_output/display_manager.py:592: RichReprWarning: Exception in _rich_repr_ while displaying object: Object of type Integer is not JSON serializable
  RichReprWarning,
Graphics3d Object

And X.show() fails (see https://groups.google.com/d/msg/sage-devel/3AnA7jlQcwA/E-mzznvHBgAJ)

It works if Sage numbers are converted into "native" Python. (e.g. int()).

Change History (10)

comment:1 Changed 15 months ago by dimpase

  • Description modified (diff)

comment:2 Changed 15 months ago by gh-mwageringel

I guess these are meant to be constructed via the function line or line3d, instead of initializing the Line class directly:

sage: X = line([(-1, 0, 0), (0, 0, 0), (1, 0, 0)])
sage: X
Launched html viewer for Graphics3d Object

As far as I know, this pattern applies to many graphics objects in Sage, such as those returned by the plot functions.

comment:3 Changed 15 months ago by dimpase

Well, the documentation of Line shows Line([(-1, 0, 0), (0, 0, 0), (1, 0, 0)]) as if it should work. So something needs to be done at least there.

comment:4 Changed 15 months ago by embray

This came up also in #27592.

My suggestion there (on which there hasn't been any action, or even a ticket) is that SageObjects should get a _json_ method, which returns a JSON serialization of that object, if possible. We then need to add a custom JSONEncoder which can handle objects with a _json_ method where possible.

Unfortunately there is no way to extend the standard json module to support new types without directly using a custom JSONEncoder, which means any code in Sage that outputs JSON needs to use our custom encoder. This is not such a problem--it just needs to be documented properly, e.g. that to JSON-encode Sage objects one needs to use sage.json or whatever instead of the plain json module.

There has been action lately on python-dev surrounding making it easier to do custom JSON serialization/deserialization but I haven't followed it lately. I don't think there's even a PEP yet.

comment:5 Changed 13 months ago by gh-jcamp0x2a

Hello! If you'll forgive my intrusion into this ticket, the changes I've made in #29227 seem to have unintentionally fixed the example code from the description and linked sage-devel post here. I ran into similar problems with Integer, Rational, and symbolic expression not being JSON serializable when running my branch through the examples in the plot3d reference manual.

Rather than creating a custom JSONEncoder and trying to enumerate and convert every possible type I might encounter, I opted to just convert values to Python's int/float/string as I placed them into the plot's new threejs_repr (that eventually goes through the json module). That way, I don't need to care what those values actually are, only what the three.js template expects them to be. As long as those types define the appropriate __int__, __float__, or __str__ methods, they should work.

Probably not the ideal solution, as it requires future maintainers to know about and remember to perform those conversions, but it solved the immediate issues. I really like the idea of a _json_ method for serialization to avoid the need for these manual conversions.

Best regards!

comment:6 Changed 13 months ago by paulmasson

  • Milestone changed from sage-9.1 to sage-duplicate/invalid/wontfix
  • Reviewers set to Paul Masson
  • Status changed from new to needs_review

I can verify that this issue is fixed by #29227. Way to go, Joshua!

comment:7 Changed 13 months ago by paulmasson

  • Keywords threejs added

comment:8 Changed 13 months ago by paulmasson

  • Status changed from needs_review to positive_review

comment:9 Changed 13 months ago by chapoton

  • Resolution set to invalid
  • Status changed from positive_review to closed

comment:10 Changed 13 months ago by chapoton

  • Resolution changed from invalid to duplicate
Note: See TracTickets for help on using tickets.