Opened 10 years ago

Closed 8 years ago

#14329 closed enhancement (fixed)

Remove all the "is_[A-Z].*" from global namespace

Reported by: Punarbasu Purkayastha Owned by: tbd
Priority: minor Milestone: sage-6.3
Component: performance Keywords:
Cc: Merged in:
Authors: Marc Mezzarobba Reviewers: Peter Bruin
Report Upstream: N/A Work issues:
Branch: b948874 (Commits, GitHub, GitLab) Commit: b9488746a27bb73eee93ad81d1bea1d0ddfec2f5
Dependencies: #15076, #15098, #15333 Stopgaps:

Status badges

Description (last modified by Marc Mezzarobba)

Remove all the is_[A-Z].* from global namespace. They have been deprecated for over two years.

step 1) remove the imports of is_* from rings/all.py (#15076)

step 2) remove the imports of is_* from matrix/all.py (#15098)

step 3) idem for modules/all.py and structure/all.py (#15333)

step 4) remove otherwise unused (i.e., not re-imported) imports of is_[A-Z]* (this ticket)

step 5) remove the remaining imports of is_[A-Z]* (this ticket)

Change History (32)

comment:1 Changed 10 years ago by Keshav Kini

Do you mean is_[A-Z][A-Za-z]*?

comment:2 Changed 10 years ago by Punarbasu Purkayastha

Description: modified (diff)
Summary: Remove all the `is_[A-Z]*` from global namespaceRemove all the "is_[A-Z].*" from global namespace

Arrgghhh! regexp!! There, the current modification should suffice.

On a serious note, what I do mean is all the "is_Vector", "is_Polynomial", "is_Matrix", and what not. The patch will be trivial but tiresome. I don't know any means of listing and testing whether they give a deprecation warning automatically. So, it has to be a slow, manual, and visual process.

comment:3 Changed 10 years ago by Keshav Kini

import re
from subprocess import check_output, STDOUT, CalledProcessError
for x in globals().keys():
    if re.match("is_[A-Z]", x) is not None:
        try:
            output = check_output(['sage', '-c', x + '(None)'], stderr=STDOUT)
        except CalledProcessError as e:
            output = e.output
        if re.match(".*DeprecationWarning", output) is not None:
            print x

is a hack but should work.

comment:4 Changed 10 years ago by Nathann Cohen

(cc me)

comment:5 Changed 9 years ago by Travis Scrimshaw

Milestone: sage-5.9sage-duplicate/invalid/wontfix
Status: newneeds_review

This is a duplicate of #12824.

comment:6 Changed 9 years ago by Punarbasu Purkayastha

I was definitely not advocating removing the functions themselves. They can still be used internally. Is it desired that the functions themselves are removed?

comment:7 Changed 9 years ago by Travis Scrimshaw

Since they've already been deprecated (from the global namespace in #10107), the trivial ones can be removed since we don't really want to use them IMO (a extra unnecessary python function call). Perhaps we could just use this ticket to explicitly remove these functions only from the global namespace (as a step towards #12824)?

comment:8 Changed 9 years ago by Punarbasu Purkayastha

Yes. That was my intention. :-)

comment:9 Changed 9 years ago by Travis Scrimshaw

Milestone: sage-duplicate/invalid/wontfixsage-5.9
Status: needs_reviewneeds_work

Ah okay, then this patch would just be a matter of finding which all.py files are importing these...

comment:10 Changed 9 years ago by Jeroen Demeyer

Milestone: sage-5.11sage-5.12

comment:11 Changed 9 years ago by Frédéric Chapoton

As a first step I propose #15076

comment:12 Changed 9 years ago by Frédéric Chapoton

Description: modified (diff)

comment:13 Changed 9 years ago by Frédéric Chapoton

Description: modified (diff)

step 1) should be ok and is wating for review at #15076

comment:14 Changed 9 years ago by Frédéric Chapoton

Description: modified (diff)

comment:15 Changed 9 years ago by Frédéric Chapoton

Description: modified (diff)

comment:16 Changed 9 years ago by Frédéric Chapoton

Description: modified (diff)

comment:17 Changed 9 years ago by Frédéric Chapoton

Description: modified (diff)

comment:18 Changed 9 years ago by For batch modifications

Milestone: sage-6.1sage-6.2

comment:19 Changed 9 years ago by Marc Mezzarobba

Branch: u/mmezzarobba/14329-remove_is_foo
Commit: 04a9dc703fec1f6facd9837afc6aff6f53335112
Dependencies: #15076, #15098, #15333
Priority: majorminor
Status: needs_workneeds_review

New commits:

865ce70trac #15333 remove import of is_* in modules/all.py
98a2153trac #15333 remove global imports in structure/all.py
04a9dc7global is_*: delete deprecation warning

comment:20 Changed 9 years ago by Marc Mezzarobba

Authors: Marc Mezzarobba

comment:21 Changed 9 years ago by Frédéric Chapoton

For a ticket like that, it is very important to test sage in full. Did you do that ?

comment:22 in reply to:  21 Changed 9 years ago by Marc Mezzarobba

Replying to chapoton:

For a ticket like that, it is very important to test sage in full. Did you do that ?

I ran make ptest, but not # long time nor optional tests (...but the few doctests matching sage:.*is_.*optional seem to properly import the predicate they call).

Last edited 9 years ago by Marc Mezzarobba (previous) (diff)

comment:23 Changed 8 years ago by For batch modifications

Milestone: sage-6.2sage-6.3

comment:24 Changed 8 years ago by Peter Bruin

Status: needs_reviewneeds_info

There are still dozens of is_* functions in the global namespace; is_Radix64StringMonoidElement, just to name a random example arising from typing is_[tab] in Sage 6.3.beta1. Shouldn't there be some more steps like 1-3 in the ticket description before we can remove the warning?

comment:25 in reply to:  24 Changed 8 years ago by Marc Mezzarobba

Authors: Marc Mezzarobba
Description: modified (diff)
Status: needs_infoneeds_work

Replying to pbruin:

There are still dozens of is_* functions in the global namespace; is_Radix64StringMonoidElement, just to name a random example arising from typing is_[tab] in Sage 6.3.beta1. Shouldn't there be some more steps like 1-3 in the ticket description before we can remove the warning?

Looks like you are completely right! I don't remember why I thought we were done.

Btw many of the imports that were supposed to be removed as part of steps 1)-3) are still there (or were added back?).

Last edited 8 years ago by Marc Mezzarobba (previous) (diff)

comment:26 Changed 8 years ago by git

Commit: 04a9dc703fec1f6facd9837afc6aff6f53335112064e29f3c722a0a56e893ec760a5c79e261e6a1b

Branch pushed to git repo; I updated commit sha1. This was a forced push. Last 10 new commits:

ef52d40rm global is_EllipticCurve
275ecb3rm global is_HyperellipticCurve
e055984rm global is_{Expect,Gap,Singular}Element
b0d2cd0rm global is_Finite_Field, is_Finite_FieldElement
9d2363drm global is_Graphics
03fb568rm global is_ProjectiveSpace
07f24dbrm global is_QuadraticForm
ccb3ac7rm global is_Scheme
da22e56rm global is_Morphism
064e29fglobal is_*: delete deprecation warning

comment:27 Changed 8 years ago by git

Commit: 064e29f3c722a0a56e893ec760a5c79e261e6a1ba616a717e1c2f4a2790b864856ccb91bdeca02d6

Branch pushed to git repo; I updated commit sha1. This was a forced push. Last 10 new commits:

9e0f8adrm global is_EllipticCurve
dd3e424rm global is_HyperellipticCurve
e71d0cbrm global is_{Expect,Gap,Singular}Element
0aa3b40rm global is_Finite_Field, is_Finite_FieldElement
b0dc1cfrm global is_Graphics
de6835brm global is_ProjectiveSpace
037eae0rm global is_QuadraticForm
5af32adrm global is_Scheme
95ddc08rm global is_Morphism
a616a71global is_*: delete deprecation warning

comment:28 Changed 8 years ago by Marc Mezzarobba

Work in progress, only partially tested, expect forced pushes!

comment:29 Changed 8 years ago by git

Commit: a616a717e1c2f4a2790b864856ccb91bdeca02d6b9488746a27bb73eee93ad81d1bea1d0ddfec2f5

Branch pushed to git repo; I updated commit sha1. This was a forced push. Last 10 new commits:

638875brm global is_EllipticCurve
e620d38rm global is_HyperellipticCurve
1932b64rm global is_{Expect,Gap,Singular}Element
3841cc7rm global is_Finite_Field, is_Finite_FieldElement
ae72be9rm global is_Graphics
f0ac47drm global is_ProjectiveSpace
bd2b01arm global is_QuadraticForm
fa34d62rm global is_Scheme
8ad8402rm global is_Morphism
b948874global is_*: delete deprecation warning

comment:30 Changed 8 years ago by Marc Mezzarobba

Authors: Marc Mezzarobba
Description: modified (diff)
Status: needs_workneeds_review

This time there are no is_[A-Z]* remaining in the global namespace and all (long) tests pass.

Note that I did not remove is_pAdic{Ring,Field} since these were not formally deprecated as far as I understand (but I would not mind removing them as well if other people agree).

comment:31 Changed 8 years ago by Peter Bruin

Reviewers: Peter Bruin
Status: needs_reviewpositive_review

I would be in favour of removing is_pAdic{Ring,Field} as well, but it's better to deprecate them first. We can do it in a future round of cleaning up the global namespace.

comment:32 Changed 8 years ago by Volker Braun

Branch: u/mmezzarobba/14329-remove_is_foob9488746a27bb73eee93ad81d1bea1d0ddfec2f5
Resolution: fixed
Status: positive_reviewclosed
Note: See TracTickets for help on using tickets.