Opened 7 years ago
Closed 7 years ago
#20219 closed enhancement (fixed)
Remove redundant functions from gen.pyx
Reported by:  Jeroen Demeyer  Owned by:  

Priority:  major  Milestone:  sage7.2 
Component:  interfaces  Keywords:  
Cc:  Luca De Feo  Merged in:  
Authors:  Jeroen Demeyer  Reviewers:  Vincent Delecroix 
Report Upstream:  N/A  Work issues:  
Branch:  82f8f65 (Commits, GitHub, GitLab)  Commit:  82f8f65eda75f789ce937ed1a0448d31a27bf643 
Dependencies:  Stopgaps: 
Description
Many functions in src/sage/libs/pari/gen.pyx
are just manual copies of autogenerated code. Remove those functions, but keep the doctests in src/sage/libs/pari/tests.py
.
Change History (25)
comment:1 Changed 7 years ago by
Branch:  → u/jdemeyer/remove_redundant_functions_from_gen_pyx 

comment:2 Changed 7 years ago by
Commit:  → a665e1789901a04693615a76334ed358a216b2f9 

comment:3 Changed 7 years ago by
Commit:  a665e1789901a04693615a76334ed358a216b2f9 → a15c0d26aff47569d001457201ef628825d71cf8 

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
a15c0d2  Remove redundant functions from gen.pyx

comment:4 Changed 7 years ago by
Commit:  a15c0d26aff47569d001457201ef628825d71cf8 → 8e3a9ab9586d2680bbc2994e897e40a13768372f 

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
8e3a9ab  Remove redundant functions from gen.pyx

comment:5 Changed 7 years ago by
Commit:  8e3a9ab9586d2680bbc2994e897e40a13768372f → e638b0d2310e4f794242d657f8758bb11aca4006 

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
e638b0d  Remove redundant functions from gen.pyx

comment:6 Changed 7 years ago by
Commit:  e638b0d2310e4f794242d657f8758bb11aca4006 → ba68a02ccc5834ce59a000d16e811db63c195edc 

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
ba68a02  Remove redundant functions from gen.pyx

comment:7 Changed 7 years ago by
Commit:  ba68a02ccc5834ce59a000d16e811db63c195edc → 130fc90a6740a449c79e487ab71cde7204dc8f62 

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
130fc90  Remove redundant functions from gen.pyx

comment:8 Changed 7 years ago by
Authors:  → Jeroen Demeyer 

comment:9 Changed 7 years ago by
Status:  new → needs_review 

comment:10 followups: 11 16 21 Changed 7 years ago by
Milestone:  sage7.1 → sage7.2 

Reviewers:  → Vincent Delecroix 
Status:  needs_review → needs_work 
There was no doctest for vecmin
, vecmax
, elltaniyama
, bnfcertify
, bnfunit
, idealval
, nfsubfields
, .Would it be possible to provide one to check that the methods exist?
The methods idealintersection
, reverse
, order
, listinsert
, listput
were simply removed. You should provide deprecation and test it.
Finally, could you change the doctest
sage: var('x') x sage: pari(x).ceil() x
to
sage: pari(SR.var('x')).ceil()
comment:11 followup: 12 Changed 7 years ago by
Replying to vdelecroix:
Finally, could you change the doctest
sage: var('x') x sage: pari(x).ceil() xto
sage: pari(SR.var('x')).ceil()
Why?
comment:12 followup: 13 Changed 7 years ago by
Replying to jdemeyer:
Replying to vdelecroix:
Finally, could you change the doctest
sage: var('x') x sage: pari(x).ceil() xto
sage: pari(SR.var('x')).ceil()Why?
Because var
is an horrible function that implicitely put x
in the namespace.
comment:13 followup: 14 Changed 7 years ago by
Replying to vdelecroix:
Because
var
is an horrible function that implicitely putx
in the namespace.
Would you agree with putting x
in the namespace explicitly:
sage: x = SR.var('x') sage: pari(x)....
comment:14 Changed 7 years ago by
Replying to jdemeyer:
Replying to vdelecroix:
Because
var
is an horrible function that implicitely putx
in the namespace.Would you agree with putting
x
in the namespace explicitly:sage: x = SR.var('x') sage: pari(x)....
I do.
comment:15 Changed 7 years ago by
Commit:  130fc90a6740a449c79e487ab71cde7204dc8f62 → 229974331e50c6aeeee524b587ad3e3b717f1e60 

Branch pushed to git repo; I updated commit sha1. New commits:
2299743  Use SR.symbol() instead var()

comment:16 followup: 19 Changed 7 years ago by
Replying to vdelecroix:
There was no doctest for
vecmin
,vecmax
,elltaniyama
,bnfcertify
,bnfunit
,idealval
,nfsubfields
, .Would it be possible to provide one to check that the methods exist?
I don't see the point. We provide the functions exposed by PARI/GP. If these functions are used in Sage library code, there is an implicit test that they work. If they are not used, I don't see why it's our business to check that PARI/GP has a vecmin()
function for example.
The methods
idealintersection
,reverse
,order
,listinsert
,listput
were simply removed. You should provide deprecation and test it.
idealintersection
and reverse
are tested in tests.py
. The last three indeed disappeared.
comment:17 Changed 7 years ago by
Commit:  229974331e50c6aeeee524b587ad3e3b717f1e60 → 0f2386ab32d486aca0a705c00d6da6965f24623e 

comment:18 Changed 7 years ago by
Status:  needs_work → needs_review 

comment:19 Changed 7 years ago by
Replying to jdemeyer:
Replying to vdelecroix:
There was no doctest for
vecmin
,vecmax
,elltaniyama
,bnfcertify
,bnfunit
,idealval
,nfsubfields
, .Would it be possible to provide one to check that the methods exist?I don't see the point. We provide the functions exposed by PARI/GP. If these functions are used in Sage library code, there is an implicit test that they work. If they are not used, I don't see why it's our business to check that PARI/GP has a
vecmin()
function for example.
But these functions used to be there. Some people might use them outside of the Sage library code. I do not claim that the code was good from the begining but that the changes should be as smooth as possible.
The methods
idealintersection
,reverse
,order
,listinsert
,listput
were simply removed. You should provide deprecation and test it.
idealintersection
andreverse
are tested intests.py
. The last three indeed disappeared.
The same remark applies.
comment:20 Changed 7 years ago by
Commit:  0f2386ab32d486aca0a705c00d6da6965f24623e → 552a445f3d1467b3db5a8b5469490bce05769df0 

Branch pushed to git repo; I updated commit sha1. New commits:
552a445  Manually add vecmax() and vecmin() functions

comment:21 Changed 7 years ago by
OK, let's look at the functions that you mentioned:
vecmin
,vecmax
: added back with doctests.elltaniyama
,bnfcertify
,nfsubfields
,idealval
: still exist, autogenerated.bnfunit
: still exists ingen.pyx
.idealintersection
,reverse
: still exist but deprecated.order
: added back as deprecated alias.listinsert
,listput
: now autogenerated and doctested implicitly inList()
.
comment:22 Changed 7 years ago by
Commit:  552a445f3d1467b3db5a8b5469490bce05769df0 → 82f8f65eda75f789ce937ed1a0448d31a27bf643 

Branch pushed to git repo; I updated commit sha1. New commits:
82f8f65  Merge tag '7.2.beta1' into t/20219/remove_redundant_functions_from_gen_pyx

comment:23 Changed 7 years ago by
Dependencies:  #20210, #20205, #20213, #20216, #20217 

comment:24 Changed 7 years ago by
Status:  needs_review → positive_review 

comment:25 Changed 7 years ago by
Branch:  u/jdemeyer/remove_redundant_functions_from_gen_pyx → 82f8f65eda75f789ce937ed1a0448d31a27bf643 

Resolution:  → fixed 
Status:  positive_review → closed 
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
Remove redundant functions from gen.pyx