Opened 4 years ago

Closed 4 years ago

#20219 closed enhancement (fixed)

Remove redundant functions from gen.pyx

Reported by: jdemeyer Owned by:
Priority: major Milestone: sage-7.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 auto-generated code. Remove those functions, but keep the doctests in src/sage/libs/pari/tests.py.

Change History (25)

comment:1 Changed 4 years ago by jdemeyer

  • Branch set to u/jdemeyer/remove_redundant_functions_from_gen_pyx

comment:2 Changed 4 years ago by git

  • Commit set to 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 4 years ago by git

  • Commit changed from a665e1789901a04693615a76334ed358a216b2f9 to a15c0d26aff47569d001457201ef628825d71cf8

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

  • Commit changed from a15c0d26aff47569d001457201ef628825d71cf8 to 8e3a9ab9586d2680bbc2994e897e40a13768372f

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

  • Commit changed from 8e3a9ab9586d2680bbc2994e897e40a13768372f to e638b0d2310e4f794242d657f8758bb11aca4006

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

  • Commit changed from e638b0d2310e4f794242d657f8758bb11aca4006 to ba68a02ccc5834ce59a000d16e811db63c195edc

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

  • Commit changed from ba68a02ccc5834ce59a000d16e811db63c195edc to 130fc90a6740a449c79e487ab71cde7204dc8f62

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 4 years ago by jdemeyer

  • Authors set to Jeroen Demeyer

comment:9 Changed 4 years ago by jdemeyer

  • Status changed from new to needs_review

comment:10 follow-ups: Changed 4 years ago by vdelecroix

  • Milestone changed from sage-7.1 to sage-7.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 ; follow-up: Changed 4 years ago by 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?

comment:12 in reply to: ↑ 11 ; follow-up: Changed 4 years ago by vdelecroix

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 ; follow-up: Changed 4 years ago by 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)....

comment:14 in reply to: ↑ 13 Changed 4 years ago by vdelecroix

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

  • Commit changed from 130fc90a6740a449c79e487ab71cde7204dc8f62 to 229974331e50c6aeeee524b587ad3e3b717f1e60

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

2299743Use SR.symbol() instead var()

comment:16 in reply to: ↑ 10 ; follow-up: Changed 4 years ago by 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.

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

  • Commit changed from 229974331e50c6aeeee524b587ad3e3b717f1e60 to 0f2386ab32d486aca0a705c00d6da6965f24623e

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 4 years ago by jdemeyer

  • Status changed from needs_work to needs_review

comment:19 in reply to: ↑ 16 Changed 4 years ago by vdelecroix

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

  • Commit changed from 0f2386ab32d486aca0a705c00d6da6965f24623e to 552a445f3d1467b3db5a8b5469490bce05769df0

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

552a445Manually add vecmax() and vecmin() functions

comment:21 in reply to: ↑ 10 Changed 4 years ago by jdemeyer

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 4 years ago by jdemeyer (previous) (diff)

comment:22 Changed 4 years ago by git

  • Commit changed from 552a445f3d1467b3db5a8b5469490bce05769df0 to 82f8f65eda75f789ce937ed1a0448d31a27bf643

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 4 years ago by jdemeyer

  • Dependencies #20210, #20205, #20213, #20216, #20217 deleted

comment:24 Changed 4 years ago by vdelecroix

  • Status changed from needs_review to positive_review

comment:25 Changed 4 years ago by vbraun

  • Branch changed from u/jdemeyer/remove_redundant_functions_from_gen_pyx to 82f8f65eda75f789ce937ed1a0448d31a27bf643
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.