Opened 7 years ago

Closed 9 months ago

Last modified 9 months ago

#17684 closed defect (fixed)

density_plot() is broken with functions involving symbolic expressions

Reported by: tmonteil Owned by:
Priority: major Milestone: sage-9.3
Component: graphics Keywords:
Cc: kcrisman Merged in:
Authors: Dave Morris Reviewers: Karl-Dieter Crisman
Report Upstream: N/A Work issues:
Branch: bb5a812 (Commits, GitHub, GitLab) Commit:
Dependencies: Stopgaps:

Status badges

Description

As reported on this ask question, the following does not work:

sage: f1(a, b) = 1 - b / a
sage: f2(a, b) = 1 - a / b
sage: def f12(a, b):
....:         if a - b < 0:
....:                 return f1(a, b)
....:         else:
....:                 return f2(a, b)
sage: density_plot(f,(1,2),(1,2))
KeyError: 'text/plain'

While the following works:

sage: f1(a, b) = 1 - b / a
sage: f2(a, b) = 1 - a / b
sage: def f12(a, b):
....:     if a - b < 0:
....:         return RDF(f1(a, b))
....:     else:
....:         return RDF(f2(a, b))
sage: density_plot(f12,(1,2),(1,2))

Attachments (1)

density_plots.pdf (283.6 KB) - added by gh-DaveWitteMorris 10 months ago.
density plot doctests

Download all attachments as: .zip

Change History (11)

comment:1 Changed 10 months ago by gh-DaveWitteMorris

  • Branch set to public/17684

comment:2 Changed 10 months ago by gh-DaveWitteMorris

  • Authors set to Dave Morris
  • Commit set to bb5a812902cd1d29ddb17989116d054222c144bb
  • Milestone changed from sage-6.5 to sage-9.3
  • Status changed from new to needs_review

New commits:

bb5a812trac 17684 density plot of symbolic values

comment:3 Changed 10 months ago by kcrisman

Nice. Unfortunately I can't upgrade my Sage currently to actually check this (quite old computer with fragile installation, based on past experience, and I can't afford to bork it), but patchbot says okay ... would you be willing to post some of the plots from the documentation (Including the tests for bug fixes, including your new one) here to verify? Then I'd be happy to provide positive review, the code should certainly be okay.


That said, the real bug here is in fast_float, because it should still be taking this function in setup_for_eval_on_grid and doing the "right" thing with it. See this admittedly bizarre Sage cell interact. Maybe we should open/search for a ticket about this error - I'm sure it comes up elsewhere, there are definitely open tickets with fast_float or fast_callable and I know some have suggested completely rewriting that code.

comment:4 Changed 10 months ago by kcrisman

(And of course if providing all that is too much to ask, that's fine too; I'm sure someone else will be able to review this, it's an obvious hotfix.)

Changed 10 months ago by gh-DaveWitteMorris

density plot doctests

comment:5 Changed 10 months ago by gh-DaveWitteMorris

I think the attachment (density_plots.pdf) has all of the plots that are in the doctests of this file. (Let me know if I missed any that you were wondering about.) However, I jpeg-compressed the file, so there are some artifacts, especially where the plot has white at an edge.

I did not find the issue of this ticket listed anywhere. (Related: ticket #5572 seems to suggest replacing fast_float with fast_callable.) I agree that the real problem is in fast_float, so probably a ticket should be opened (or a comment added to #5572). I think that fix will take more care, because RDF works on this ticket, but I suspect that fast_float will need to consider the precision of its inputs before choosing the field of floating-point numbers to use.

comment:6 Changed 10 months ago by kcrisman

  • Reviewers set to Karl-Dieter Crisman
  • Status changed from needs_review to positive_review

Nice, thank you very much!

Yes, go ahead and make a comment there at least - that is one of the tickets I was thinking of.

comment:7 Changed 10 months ago by gh-DaveWitteMorris

Thanks for the reviews. I added a comment to the description of #5572.

comment:8 Changed 9 months ago by vbraun

  • Branch changed from public/17684 to bb5a812902cd1d29ddb17989116d054222c144bb
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:9 Changed 9 months ago by slelievre

  • Commit bb5a812902cd1d29ddb17989116d054222c144bb deleted

Thanks for fixing this. Is the problem reported in Ask Sage question 55824 related?

comment:10 Changed 9 months ago by gh-DaveWitteMorris

It may be a similar issue, but I don't understand what is going on there. I opened #31488.

Note: See TracTickets for help on using tickets.