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:  sage9.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: 
Description (last modified by )
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/sagesupport/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
Branch:  → public/manifolds/expression_nice33399 

Cc:  Travis Scrimshaw added 
Commit:  → 70729d9b5cd69158e1b41d9a74ab1b8b03917c58 
Status:  new → needs_review 
comment:2 Changed 9 months ago by
Description:  modified (diff) 

comment:3 Changed 9 months ago by
Summary:  Bug in ExpressionNIce with composite variables → Bug in ExpressionNice with composite variables 

comment:4 Changed 9 months ago by
Reviewers:  → Matthias Koeppe 

Status:  needs_review → positive_review 
comment:5 Changed 9 months ago by
Commit:  70729d9b5cd69158e1b41d9a74ab1b8b03917c58 → 9b9b65bdcdc0685b89fff0e6a8f393c9c9e4d3e2 

Status:  positive_review → needs_review 
Branch pushed to git repo; I updated commit sha1 and set ticket back to needs_review. New commits:
9b9b65b  Remove import re from ExpressionNice (#33399)

comment:6 Changed 9 months ago by
Thank you for the review. Here is a new version, which avoids import re
.
comment:7 Changed 9 months ago by
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 followup: 9 Changed 9 months ago by
Status:  needs_review → positive_review 

This version is also fine. Note though that re
*is* part of the Python standard library.
comment:9 Changed 9 months ago by
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
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
Branch:  public/manifolds/expression_nice33399 → 9b9b65bdcdc0685b89fff0e6a8f393c9c9e4d3e2 

Resolution:  → fixed 
Status:  positive_review → closed 
New commits:
Fix bug in ExpressionNice (#33399)