#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: |
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)
Change History (11)
comment:1 Changed 2 months ago by
- Branch set to public/17684
comment:2 Changed 2 months ago by
- Commit set to bb5a812902cd1d29ddb17989116d054222c144bb
- Milestone changed from sage-6.5 to sage-9.3
- Status changed from new to needs_review
comment:3 Changed 2 months ago by
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 2 months ago by
(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.)
comment:5 Changed 2 months ago by
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 2 months ago by
- 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 2 months ago by
Thanks for the reviews. I added a comment to the description of #5572.
comment:8 Changed 6 weeks ago by
- Branch changed from public/17684 to bb5a812902cd1d29ddb17989116d054222c144bb
- Resolution set to fixed
- Status changed from positive_review to closed
comment:9 Changed 6 weeks ago by
- Commit bb5a812902cd1d29ddb17989116d054222c144bb deleted
Thanks for fixing this. Is the problem reported in Ask Sage question 55824 related?
comment:10 Changed 5 weeks ago by
It may be a similar issue, but I don't understand what is going on there. I opened #31488.
New commits:
trac 17684 density plot of symbolic values