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: sage-7.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:

Status badges

Description

Many functions in src/sage/libs/pari/gen.pyx are just manual copies of auto-generated 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 Jeroen Demeyer

Branch: u/jdemeyer/remove_redundant_functions_from_gen_pyx

comment:2 Changed 7 years ago by git

Commit: a665e1789901a04693615a76334ed358a216b2f9

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

a665e17Remove redundant functions from gen.pyx

comment:3 Changed 7 years ago by git

Commit: a665e1789901a04693615a76334ed358a216b2f9a15c0d26aff47569d001457201ef628825d71cf8

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

a15c0d2Remove redundant functions from gen.pyx

comment:4 Changed 7 years ago by git

Commit: a15c0d26aff47569d001457201ef628825d71cf88e3a9ab9586d2680bbc2994e897e40a13768372f

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

8e3a9abRemove redundant functions from gen.pyx

comment:5 Changed 7 years ago by git

Commit: 8e3a9ab9586d2680bbc2994e897e40a13768372fe638b0d2310e4f794242d657f8758bb11aca4006

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

e638b0dRemove redundant functions from gen.pyx

comment:6 Changed 7 years ago by git

Commit: e638b0d2310e4f794242d657f8758bb11aca4006ba68a02ccc5834ce59a000d16e811db63c195edc

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

ba68a02Remove redundant functions from gen.pyx

comment:7 Changed 7 years ago by git

Commit: ba68a02ccc5834ce59a000d16e811db63c195edc130fc90a6740a449c79e487ab71cde7204dc8f62

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

130fc90Remove redundant functions from gen.pyx

comment:8 Changed 7 years ago by Jeroen Demeyer

Authors: Jeroen Demeyer

comment:9 Changed 7 years ago by Jeroen Demeyer

Status: newneeds_review

comment:10 Changed 7 years ago by Vincent Delecroix

Milestone: sage-7.1sage-7.2
Reviewers: Vincent Delecroix
Status: needs_reviewneeds_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 ; Changed 7 years ago by Jeroen Demeyer

Replying to vdelecroix:

Finally, could you change the doctest

     sage: var('x')
     x
     sage: pari(x).ceil()
     x

to

    sage: pari(SR.var('x')).ceil()

Why?

comment:12 in reply to:  11 ; Changed 7 years ago by Vincent Delecroix

Replying to jdemeyer:

Replying to vdelecroix:

Finally, could you change the doctest

     sage: var('x')
     x
     sage: pari(x).ceil()
     x

to

    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 ; Changed 7 years ago by Jeroen Demeyer

Replying to vdelecroix:

Because var is an horrible function that implicitely put x 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 7 years ago by Vincent Delecroix

Replying to jdemeyer:

Replying to vdelecroix:

Because var is an horrible function that implicitely put x 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 git

Commit: 130fc90a6740a449c79e487ab71cde7204dc8f62229974331e50c6aeeee524b587ad3e3b717f1e60

Branch pushed to git repo; I updated commit sha1. New commits:

2299743Use SR.symbol() instead var()

comment:16 in reply to:  10 ; Changed 7 years ago by Jeroen Demeyer

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 git

Commit: 229974331e50c6aeeee524b587ad3e3b717f1e600f2386ab32d486aca0a705c00d6da6965f24623e

Branch pushed to git repo; I updated commit sha1. New commits:

6acc79aAdd back order() as deprecated alias of multiplicative_order()
0f2386aFix basic t_LIST functionality

comment:18 Changed 7 years ago by Jeroen Demeyer

Status: needs_workneeds_review

comment:19 in reply to:  16 Changed 7 years ago by Vincent Delecroix

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 and reverse are tested in tests.py. The last three indeed disappeared.

The same remark applies.

comment:20 Changed 7 years ago by git

Commit: 0f2386ab32d486aca0a705c00d6da6965f24623e552a445f3d1467b3db5a8b5469490bce05769df0

Branch pushed to git repo; I updated commit sha1. New commits:

552a445Manually add vecmax() and vecmin() functions

comment:21 in reply to:  10 Changed 7 years ago by Jeroen Demeyer

OK, let's look at the functions that you mentioned:

  • vecmin, vecmax: added back with doctests.
  • elltaniyama, bnfcertify, nfsubfields, idealval: still exist, auto-generated.
  • bnfunit: still exists in gen.pyx.
  • idealintersection, reverse: still exist but deprecated.
  • order: added back as deprecated alias.
  • listinsert, listput: now auto-generated and doctested implicitly in List().
Last edited 7 years ago by Jeroen Demeyer (previous) (diff)

comment:22 Changed 7 years ago by git

Commit: 552a445f3d1467b3db5a8b5469490bce05769df082f8f65eda75f789ce937ed1a0448d31a27bf643

Branch pushed to git repo; I updated commit sha1. New commits:

82f8f65Merge tag '7.2.beta1' into t/20219/remove_redundant_functions_from_gen_pyx

comment:23 Changed 7 years ago by Jeroen Demeyer

Dependencies: #20210, #20205, #20213, #20216, #20217

comment:24 Changed 7 years ago by Vincent Delecroix

Status: needs_reviewpositive_review

comment:25 Changed 7 years ago by Volker Braun

Branch: u/jdemeyer/remove_redundant_functions_from_gen_pyx82f8f65eda75f789ce937ed1a0448d31a27bf643
Resolution: fixed
Status: positive_reviewclosed
Note: See TracTickets for help on using tickets.