Opened 9 months ago

Closed 9 months ago

#33399 closed defect (fixed)

Bug in ExpressionNice with composite variables

Reported by: Eric Gourgoulhon Owned by:
Priority: major Milestone: sage-9.6
Component: manifolds Keywords:
Cc: Travis Scrimshaw Merged in:
Authors: Eric Gourgoulhon Reviewers: Matthias Koeppe
Report Upstream: N/A Work issues:
Branch: 9b9b65b (Commits, GitHub, GitLab) Commit: 9b9b65bdcdc0685b89fff0e6a8f393c9c9e4d3e2
Dependencies: Stopgaps:

Status badges

Description (last modified by Eric Gourgoulhon)

In Sage 9.6.beta2, we have

sage: from sage.manifolds.utilities import ExpressionNice
sage: u, v = var('u v')
sage: f = function('F')(u + v)
sage: f
F(u + v)
sage: ExpressionNice(diff(f, u))
d(F)/d(u + v)

So far, so good, but

sage: f = function('F')(u - v)
sage: ExpressionNice(diff(f, u))
d(F)/du - v

This bug has been reported in https://groups.google.com/g/sage-support/c/fbE0APqThEk

With the fix introduced in this ticket, the last output is now

d(F)/d(u - v)

Change History (11)

comment:1 Changed 9 months ago by Eric Gourgoulhon

Branch: public/manifolds/expression_nice-33399
Cc: Travis Scrimshaw added
Commit: 70729d9b5cd69158e1b41d9a74ab1b8b03917c58
Status: newneeds_review

New commits:

70729d9Fix bug in ExpressionNice (#33399)

comment:2 Changed 9 months ago by Eric Gourgoulhon

Description: modified (diff)

comment:3 Changed 9 months ago by Eric Gourgoulhon

Summary: Bug in ExpressionNIce with composite variablesBug in ExpressionNice with composite variables

comment:4 Changed 9 months ago by Matthias Köppe

Reviewers: Matthias Koeppe
Status: needs_reviewpositive_review

comment:5 Changed 9 months ago by git

Commit: 70729d9b5cd69158e1b41d9a74ab1b8b03917c589b9b65bdcdc0685b89fff0e6a8f393c9c9e4d3e2
Status: positive_reviewneeds_review

Branch pushed to git repo; I updated commit sha1 and set ticket back to needs_review. New commits:

9b9b65bRemove import re from ExpressionNice (#33399)

comment:6 Changed 9 months ago by Eric Gourgoulhon

Thank you for the review. Here is a new version, which avoids import re.

comment:7 Changed 9 months ago by Eric Gourgoulhon

What do you think about the new version? Sorry for having pushed it after your positive review, but I thought this rewriting was better than the quick fix I first submitted, especially because it relies on standard Python, without the need of importing the re module.

comment:8 Changed 9 months ago by Matthias Köppe

Status: needs_reviewpositive_review

This version is also fine. Note though that re *is* part of the Python standard library.

comment:9 in reply to:  8 Changed 9 months ago by Eric Gourgoulhon

Replying to mkoeppe:

This version is also fine.

Thank you!

Note though that re *is* part of the Python standard library.

You are right; I should have said basic Python, instead of standard Python. My concern was actually the CPU performance: it's always better to avoid an import, isn't it? (unless of course the function you import does the job more efficiently).

comment:10 Changed 9 months ago by Matthias Köppe

This module is already widely used by pretty much everything, so it's likely to be already loaded. And it's likely to be faster than doing the loop over the characters in the new version. But I don't think it matters.

comment:11 Changed 9 months ago by Volker Braun

Branch: public/manifolds/expression_nice-333999b9b65bdcdc0685b89fff0e6a8f393c9c9e4d3e2
Resolution: fixed
Status: positive_reviewclosed
Note: See TracTickets for help on using tickets.