Bug in ExpressionNice with composite variables

[egourgoulhon]

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)

CC: @tscrim

Component: manifolds

Author: Eric Gourgoulhon

Branch/Commit: 9b9b65b

Reviewer: Matthias Koeppe

Issue created by migration from https://trac.sagemath.org/ticket/33399

[egourgoulhon]

Branch: public/manifolds/expression_nice-33399

[egourgoulhon]

Commit: 70729d9

[egourgoulhon]

New commits:

70729d9

Fix bug in ExpressionNice (#33399)

[mkoeppe]

Reviewer: Matthias Koeppe

[sagetrac-git]

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)

[sagetrac-git]

Changed commit from 70729d9 to 9b9b65b

[egourgoulhon]

comment:6

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

[egourgoulhon]

comment:7

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.

[mkoeppe]

comment:8

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

[egourgoulhon]

comment:9

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).

[mkoeppe]

comment:10

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.

[vbraun]

Changed branch from public/manifolds/expression_nice-33399 to 9b9b65b