Opened 6 years ago

Closed 5 years ago

#14329 closed enhancement (fixed)

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

Reported by: ppurka 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) Commit: b9488746a27bb73eee93ad81d1bea1d0ddfec2f5
Dependencies: #15076, #15098, #15333 Stopgaps:

Description (last modified by mmezzarobba)

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 6 years ago by kini

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

comment:2 Changed 6 years ago by ppurka

  • Description modified (diff)
  • Summary changed from Remove all the `is_[A-Z]*` from global namespace to Remove 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 6 years ago by 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 6 years ago by ncohen

(cc me)

comment:5 Changed 6 years ago by tscrim

  • Milestone changed from sage-5.9 to sage-duplicate/invalid/wontfix
  • Status changed from new to needs_review

This is a duplicate of #12824.

comment:6 Changed 6 years ago by ppurka

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 6 years ago by tscrim

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 6 years ago by ppurka

Yes. That was my intention. :-)

comment:9 Changed 6 years ago by tscrim

  • Milestone changed from sage-duplicate/invalid/wontfix to sage-5.9
  • Status changed from needs_review to needs_work

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

comment:10 Changed 6 years ago by jdemeyer

  • Milestone changed from sage-5.11 to sage-5.12

comment:11 Changed 6 years ago by chapoton

As a first step I propose #15076

comment:12 Changed 6 years ago by chapoton

  • Description modified (diff)

comment:13 Changed 6 years ago by chapoton

  • Description modified (diff)

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

comment:14 Changed 6 years ago by chapoton

  • Description modified (diff)

comment:15 Changed 6 years ago by chapoton

  • Description modified (diff)

comment:16 Changed 6 years ago by chapoton

  • Description modified (diff)

comment:17 Changed 6 years ago by chapoton

  • Description modified (diff)

comment:18 Changed 6 years ago by vbraun_spam

  • Milestone changed from sage-6.1 to sage-6.2

comment:19 Changed 6 years ago by mmezzarobba

  • Branch set to u/mmezzarobba/14329-remove_is_foo
  • Commit set to 04a9dc703fec1f6facd9837afc6aff6f53335112
  • Dependencies set to #15076, #15098, #15333
  • Priority changed from major to minor
  • Status changed from needs_work to needs_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 6 years ago by mmezzarobba

  • Authors set to Marc Mezzarobba

comment:21 follow-up: Changed 6 years ago by 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 6 years ago by mmezzarobba

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 6 years ago by mmezzarobba (previous) (diff)

comment:23 Changed 5 years ago by vbraun_spam

  • Milestone changed from sage-6.2 to sage-6.3

comment:24 follow-up: Changed 5 years ago by pbruin

  • Status changed from needs_review to needs_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 5 years ago by mmezzarobba

  • Authors Marc Mezzarobba deleted
  • Description modified (diff)
  • Status changed from needs_info to needs_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 5 years ago by mmezzarobba (previous) (diff)

comment:26 Changed 5 years ago by git

  • Commit changed from 04a9dc703fec1f6facd9837afc6aff6f53335112 to 064e29f3c722a0a56e893ec760a5c79e261e6a1b

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 5 years ago by git

  • Commit changed from 064e29f3c722a0a56e893ec760a5c79e261e6a1b to a616a717e1c2f4a2790b864856ccb91bdeca02d6

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 5 years ago by mmezzarobba

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

comment:29 Changed 5 years ago by git

  • Commit changed from a616a717e1c2f4a2790b864856ccb91bdeca02d6 to b9488746a27bb73eee93ad81d1bea1d0ddfec2f5

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 5 years ago by mmezzarobba

  • Authors set to Marc Mezzarobba
  • Description modified (diff)
  • Status changed from needs_work to needs_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 5 years ago by pbruin

  • Reviewers set to Peter Bruin
  • Status changed from needs_review to positive_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 5 years ago by vbraun

  • Branch changed from u/mmezzarobba/14329-remove_is_foo to b9488746a27bb73eee93ad81d1bea1d0ddfec2f5
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.