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

Priority:  major  Milestone:  sage7.2 
Component:  interfaces  Keywords:  
Cc:  defeo  Merged in:  
Authors:  Jeroen Demeyer  Reviewers:  Vincent Delecroix 
Report Upstream:  N/A  Work issues:  
Branch:  82f8f65 (Commits)  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 5 years ago by
 Branch set to u/jdemeyer/remove_redundant_functions_from_gen_pyx
comment:2 Changed 5 years ago by
 Commit set to a665e1789901a04693615a76334ed358a216b2f9
comment:3 Changed 5 years ago by
 Commit changed from a665e1789901a04693615a76334ed358a216b2f9 to 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 5 years ago by
 Commit changed from a15c0d26aff47569d001457201ef628825d71cf8 to 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 5 years ago by
 Commit changed from 8e3a9ab9586d2680bbc2994e897e40a13768372f to 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 5 years ago by
 Commit changed from e638b0d2310e4f794242d657f8758bb11aca4006 to 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 5 years ago by
 Commit changed from ba68a02ccc5834ce59a000d16e811db63c195edc to 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 5 years ago by
comment:9 Changed 5 years ago by
 Status changed from new to needs_review
comment:10 followups: ↓ 11 ↓ 16 ↓ 21 Changed 5 years ago by
 Milestone changed from sage7.1 to sage7.2
 Reviewers set to Vincent Delecroix
 Status changed from needs_review to 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 in reply to: ↑ 10 ; followup: ↓ 12 Changed 5 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 in reply to: ↑ 11 ; followup: ↓ 13 Changed 5 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 in reply to: ↑ 12 ; followup: ↓ 14 Changed 5 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 in reply to: ↑ 13 Changed 5 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 5 years ago by
 Commit changed from 130fc90a6740a449c79e487ab71cde7204dc8f62 to 229974331e50c6aeeee524b587ad3e3b717f1e60
Branch pushed to git repo; I updated commit sha1. New commits:
2299743  Use SR.symbol() instead var()

comment:16 in reply to: ↑ 10 ; followup: ↓ 19 Changed 5 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 5 years ago by
 Commit changed from 229974331e50c6aeeee524b587ad3e3b717f1e60 to 0f2386ab32d486aca0a705c00d6da6965f24623e
comment:18 Changed 5 years ago by
 Status changed from needs_work to needs_review
comment:19 in reply to: ↑ 16 Changed 5 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 5 years ago by
 Commit changed from 0f2386ab32d486aca0a705c00d6da6965f24623e to 552a445f3d1467b3db5a8b5469490bce05769df0
Branch pushed to git repo; I updated commit sha1. New commits:
552a445  Manually add vecmax() and vecmin() functions

comment:21 in reply to: ↑ 10 Changed 5 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 5 years ago by
 Commit changed from 552a445f3d1467b3db5a8b5469490bce05769df0 to 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 5 years ago by
 Dependencies #20210, #20205, #20213, #20216, #20217 deleted
comment:24 Changed 5 years ago by
 Status changed from needs_review to positive_review
comment:25 Changed 5 years ago by
 Branch changed from u/jdemeyer/remove_redundant_functions_from_gen_pyx to 82f8f65eda75f789ce937ed1a0448d31a27bf643
 Resolution set to fixed
 Status changed from positive_review to closed
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
Remove redundant functions from gen.pyx