Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
ppurka opened this issue Mar 21, 2013 · 42 comments
Closed

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

ppurka opened this issue Mar 21, 2013 · 42 comments

Comments

@ppurka
Copy link
Member

ppurka commented Mar 21, 2013

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)

Depends on #15076
Depends on #15098
Depends on #15333

Component: performance

Author: Marc Mezzarobba

Branch/Commit: b948874

Reviewer: Peter Bruin

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

@ppurka ppurka added this to the sage-5.9 milestone Mar 21, 2013
@kini
Copy link
Contributor

kini commented Mar 22, 2013

comment:1

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

@ppurka
Copy link
Member Author

ppurka commented Mar 22, 2013

comment:2

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.

@ppurka

This comment has been minimized.

@ppurka ppurka changed the title Remove all the is_[A-Z]* from global namespace Remove all the "is_[A-Z].*" from global namespace Mar 22, 2013
@kini
Copy link
Contributor

kini commented Mar 22, 2013

comment:3
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.

@nathanncohen
Copy link
Mannequin

nathanncohen mannequin commented Mar 26, 2013

comment:4

(cc me)

@tscrim
Copy link
Collaborator

tscrim commented Apr 2, 2013

comment:5

This is a duplicate of #12824.

@tscrim tscrim removed this from the sage-5.9 milestone Apr 2, 2013
@ppurka
Copy link
Member Author

ppurka commented Apr 2, 2013

comment:6

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

@tscrim
Copy link
Collaborator

tscrim commented Apr 2, 2013

comment:7

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

@ppurka
Copy link
Member Author

ppurka commented Apr 3, 2013

comment:8

Yes. That was my intention. :-)

@tscrim
Copy link
Collaborator

tscrim commented Apr 3, 2013

comment:9

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

@tscrim tscrim added this to the sage-5.9 milestone Apr 3, 2013
@jdemeyer jdemeyer modified the milestones: sage-5.11, sage-5.12 Aug 13, 2013
@fchapoton
Copy link
Contributor

comment:11

As a first step I propose #15076

@fchapoton

This comment has been minimized.

@fchapoton
Copy link
Contributor

comment:13

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

@fchapoton

This comment has been minimized.

@fchapoton

This comment has been minimized.

@fchapoton

This comment has been minimized.

@fchapoton

This comment has been minimized.

@fchapoton

This comment has been minimized.

@pjbruin
Copy link
Contributor

pjbruin commented May 15, 2014

comment:24

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?

@mezzarobba

This comment has been minimized.

@mezzarobba
Copy link
Member

comment:25

Replying to @pjbruin:

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

@mezzarobba
Copy link
Member

Changed author from Marc Mezzarobba to none

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented May 16, 2014

Changed commit from 04a9dc7 to 064e29f

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented May 16, 2014

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

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented May 16, 2014

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

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented May 16, 2014

Changed commit from 064e29f to a616a71

@mezzarobba
Copy link
Member

comment:28

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

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented May 17, 2014

Changed commit from a616a71 to b948874

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented May 17, 2014

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

@mezzarobba
Copy link
Member

Author: Marc Mezzarobba

@mezzarobba

This comment has been minimized.

@mezzarobba
Copy link
Member

comment:30

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

@pjbruin
Copy link
Contributor

pjbruin commented May 17, 2014

Reviewer: Peter Bruin

@pjbruin
Copy link
Contributor

pjbruin commented May 17, 2014

comment:31

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.

@vbraun
Copy link
Member

vbraun commented May 21, 2014

Changed branch from u/mmezzarobba/14329-remove_is_foo to b948874

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

8 participants