var('x',ns=False) -- should go boom but silently gives a new symbolic variable

[williamstein]

sage: type(var('x',ns=False))
<type 'sage.symbolic.expression.Expression'>

Component: calculus

Author: Karl-Dieter Crisman

Reviewer: Jason Grout, Burcin Erocal

Merged: Sage 4.1.2.alpha4

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

[williamstein]

comment:1

The fix should be to raise a DeprecationError... or possibly just a NotImplementedError...

[kcrisman]

comment:2

This raises a NotImplementedError for ns=False, but still creates the variable for ns=1 or ns=True, with a verbose level 0 message.

[golam-m-hossain]

comment:3

Patch at #6559 enhances symbolic variables definition. Unfortunately, the patch there
will conflict with the patch here.

[kcrisman]

comment:4

It looks like #6559 functionality is better to incorporate first. What happens after its inclusion with the following?

sage: var('z',ns=False)

sage: var('z',ns=True)

The results of these will help create a new patch, though that may not happen for a bit.

Alternately, since this one is small, one could review it positively (if it deserves to be) :) and then base the bigger patch at #6559 on it.

[kcrisman]

Based on 4.1.1 and #6559

[kcrisman]

comment:5

Attachment: trac_6340-var-ns-based-6559.patch.gz

Depending on which one is reviewed first, here's a patch on top of #6559. Should work identically.

[jasongrout]

comment:6

This should use the deprecation function instead of the verbose function.

For example (from matrix_rational_dense.pyx)

        from sage.misc.misc import deprecation
        deprecation("'invert' is deprecated; use 'inverse' instead.")

[burcin]

comment:7

I think NotImplementedError is OK for ns=False, but we should use a deprecation warning for ns=1.

[kcrisman]

comment:8

This makes sense. I've updated the first patch as per Burcin's idea, which seems most appropriate.

[kcrisman]

Author: Karl-Dieter Crisman

[burcin]

comment:9

Sorry for not pointing this out earlier, but I suggest changing the block:

    if ('ns', False) in kwds.items(): 
        raise NotImplementedError, "The new (Pynac) symbolics are now the only symbolics; please do not use keyword `ns` any longer." 
    if ('ns', True) in kwds.items(): 
        from sage.misc.misc import deprecation 
        deprecation("The new (Pynac) symbolics are now the only symbolics; please do not use keyword 'ns' any longer.") 

with

    if kwds.has_key('ns'):
        if kwds['ns']:
            from sage.misc.misc import deprecation 
            deprecation("The new (Pynac) symbolics are now the only symbolics; please do not use keyword 'ns' any longer.") 
        else:
            raise NotImplementedError, "The new (Pynac) symbolics are now the only symbolics; please do not use keyword `ns` any longer." 

Even if kwds is expected to be empty, it is a waste to call .items().

Putting a check that *args is empty would also help. Dropping arguments silently is not very user friendly.

[kcrisman]

comment:10

Yes, I knew there was a more elegant way to do it, but didn't have time to look it up. As for *args, I think I can safely get rid of that completely, since there are no args, only keywords. Patch coming up.

[kcrisman]

Based on 4.1.2.alpha2

[kcrisman]

comment:11

Attachment: trac_6340-var-ns.patch.gz

This should take care of it, I hope.

[burcin]

Attachment: trac_6340-missing_bits.patch.gz

more doctest fixes

[burcin]

Reviewer: Jason Grout, Burcin Erocal

[burcin]

comment:12

Looks good to me. AFAICT, there were two more places using the ns=1 option. attachment: trac_6340-missing_bits.patch should take care of them.

Apply only

• attachment: trac_6340-var-ns.patch
• attachment: trac_6340-missing_bits.patch

[kcrisman]

comment:13

To release manager: the "missing bits" may be covered in other patches reviewed related to symbolics, so do not merge if that one won't merge (simple enough!).

[sagetrac-mvngu]

comment:14

Merged trac_6340-var-ns.patch.

[sagetrac-mvngu]

Merged: Sage 4.1.2.alpha3

[sagetrac-mvngu]

Changed merged from Sage 4.1.2.alpha3 to Sage 4.1.2.alpha4

[sagetrac-mvngu]

comment:15

There is no 4.1.2.alpha3. Sage 4.1.2.alpha3 was William Stein's release for working on making the notebook a standalone package.