Opened 7 years ago

Closed 7 years ago

Last modified 7 years ago

#17631 closed enhancement (fixed)

Auto-generate gen.pyx -- part 1

Reported by: jdemeyer Owned by:
Priority: major Milestone: sage-6.5
Component: interfaces Keywords:
Cc: vdelecroix Merged in:
Authors: Jeroen Demeyer Reviewers: Vincent Delecroix
Report Upstream: N/A Work issues:
Branch: b880274 (Commits, GitHub, GitLab) Commit:
Dependencies: #16997 Stopgaps:

Status badges

Description (last modified by jdemeyer)

The files src/sage/libs/pari/gen.pyx and src/sage/libs/pari/pari_instance.pyx can be mostly auto-generated.

This first ticket introduces the infrastructure (except for documentation) and removes the manually-defined functions from gen.pyx and pari_instance.pyx only in trivial cases (function does the same thing and no documentation gets lost).

Follow-up: #17860

Change History (31)

comment:1 Changed 7 years ago by jdemeyer

  • Dependencies set to #16997

comment:2 Changed 7 years ago by vdelecroix

  • Cc vdelecroix added

comment:3 Changed 7 years ago by jdemeyer

  • Branch set to u/jdemeyer/ticket/17631
  • Created changed from 01/14/15 12:45:43 to 01/14/15 12:45:43
  • Modified changed from 01/14/15 16:21:02 to 01/14/15 16:21:02

comment:4 Changed 7 years ago by git

  • Commit set to a18d97fbe8c4a469c470eac06c143fc2a2deca41

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

a18d97fAuto-generate parts of gen.pyx and pari_instance.pyx

comment:5 Changed 7 years ago by jdemeyer

  • Description modified (diff)
  • Summary changed from Auto-generate gen.pyx to Auto-generate gen.pyx -- part 1

comment:6 Changed 7 years ago by git

  • Commit changed from a18d97fbe8c4a469c470eac06c143fc2a2deca41 to 58af466e7c33917b3fdcbf45449a918a1f0df761

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

58af466Auto-generate parts of gen.pyx and pari_instance.pyx

comment:7 Changed 7 years ago by git

  • Commit changed from 58af466e7c33917b3fdcbf45449a918a1f0df761 to fd9074ffb0b8ddd94170bdb0ff652b0764dd9a81

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

ade4f75Upgrade PARI to git master
fd9074fAuto-generate parts of gen.pyx and pari_instance.pyx

comment:8 Changed 7 years ago by jdemeyer

  • Status changed from new to needs_review

comment:9 follow-ups: Changed 7 years ago by vdelecroix

Is it on purpose that decl.pxi and declinl.pxi not automatically generated as well from 'paridecl.h'? It seems that we even have a parisage.h in between! Is the following in parisage.h still true

#undef coeff  /* Conflicts with NTL */

I made some review commits. You can cherry-pick the one you want:

  • commit 1 and 4 are trivial
  • Don't we want to keep the order of the pari.desc file? It would be enough to make read_pari_desc an iterator and avoid the sorted in PariFunctionGenerator.__call__. (see commit 3)
  • It is not very nice to hardcode the path of pari.desc since some distro might want to build with their own pari. The solution is not perfect but at least the pari source is only hardcoded in one place. (see commit 2)

comment:10 Changed 7 years ago by jdemeyer

Where are the review commits?

comment:11 Changed 7 years ago by vdelecroix

  • Branch changed from u/jdemeyer/ticket/17631 to u/uvdelecroix/17631
  • Commit fd9074ffb0b8ddd94170bdb0ff652b0764dd9a81 deleted

here they are

comment:12 Changed 7 years ago by vdelecroix

  • Branch changed from u/uvdelecroix/17631 to u/vdelecroix/17631
  • Commit set to 51236663e6488db63506102850bc69628a16764a

New commits:

ade4f75Upgrade PARI to git master
fd9074fAuto-generate parts of gen.pyx and pari_instance.pyx
82a67fctrac #17631 (reviewer commit 1): add a force argument
8da123atrac #17631 (reviewer commit 2): pari_src function
eb5a737trac #17631 (reviewer commit 3): make read_pari_desc an iterator
5123666trac #17631 (reviewer commit 3): simpler read_decl

comment:13 in reply to: ↑ 9 Changed 7 years ago by jdemeyer

Replying to vdelecroix:

  • Don't we want to keep the order of the pari.desc file? It would be enough to make read_pari_desc an iterator and avoid the sorted in PariFunctionGenerator.__call__. (see commit 3)

I don't think the order really matters. When developing this code, I did find it nice that I could do D["bnfinit"]. So I prefer to not make this change.

I agree (modulo a few details) with the other commits.

comment:14 Changed 7 years ago by jdemeyer

  • Branch changed from u/vdelecroix/17631 to u/jdemeyer/17631

comment:15 Changed 7 years ago by jdemeyer

  • Commit changed from 51236663e6488db63506102850bc69628a16764a to 98022fd94d8483a4b7f2b19af598bc8c1d3eb1dc
  • Reviewers set to Vincent Delecroix

New commits:

b1b29ebtrac #17631 (reviewer commit 3): simpler read_decl
98022fdRename pari_src() to pari_share()

comment:16 in reply to: ↑ 9 Changed 7 years ago by jdemeyer

Replying to vdelecroix:

Is it on purpose that decl.pxi and declinl.pxi not automatically generated as well from 'paridecl.h'?

Doing that would be hard. Not impossible, but a highly non-trivial project.

It seems that we even have a parisage.h in between! Is the following in parisage.h still true

#undef coeff  /* Conflicts with NTL */

Yes, we should leave that.

comment:17 Changed 7 years ago by jdemeyer

  • Description modified (diff)

comment:18 Changed 7 years ago by git

  • Commit changed from 98022fd94d8483a4b7f2b19af598bc8c1d3eb1dc to a5a6b25ad3c322c03cec85b6c85da4fcb5508cdf

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

a5a6b25Merge tag '6.6.beta4' into t/17631/17631

comment:19 follow-up: Changed 7 years ago by vdelecroix

I am a bit worried since the auto generated files do not get any doctest in them...

What is that in src/sage/libs/pari/pari_instance.pyx?

+            #from sage.misc.superseded import deprecation
+            #deprecation(XXXXX, 'passing -1 as PARI variable is deprecated, use None instead')

Why did you keep so much methods in the gen class in gen.pyx (such as List, Mat, sign, vecmin, vecmax, etc)?

Where can I found the implementation and/or documentation of pari_catch_sig_on and co?

comment:20 Changed 7 years ago by vdelecroix

  • Status changed from needs_review to needs_info

comment:21 in reply to: ↑ 19 ; follow-ups: Changed 7 years ago by jdemeyer

Replying to vdelecroix:

I am a bit worried since the auto generated files do not get any doctest in them...

That's true. On the other hand, I did not remove any doctests either, so it's a least status quo. PARI itself has quite an extensive test suite (run if you install PARI with SAGE_CHECK=yes or with ./sage -i -c pari).

What is that in src/sage/libs/pari/pari_instance.pyx?

+            #from sage.misc.superseded import deprecation
+            #deprecation(XXXXX, 'passing -1 as PARI variable is deprecated, use None instead')

It's basically a reminder to myself that this really should be deprecated.

Why did you keep so much methods in the gen class in gen.pyx (such as List, Mat, sign, vecmin, vecmax, etc)?

In any case, I didn't want to remove any doctests. I also didn't check very carefully which methods could be removed.

Where can I found the implementation and/or documentation of pari_catch_sig_on and co?

See src/sage/libs/pari/pari_err.pxi. But pari_catch_sig_on() and pari_catch_sig_off are just sig_on() and sig_off. This used to be different before we had #14894.

Most of your comments are indeed good comments, but my first priority was to get the general framework (i.e. this ticket) reviewed, we can still improve things later. I prefer to split tickets in smaller pieces, I dislike these huge tickets which do everything.

comment:22 in reply to: ↑ 21 ; follow-up: Changed 7 years ago by vdelecroix

Replying to jdemeyer:

Replying to vdelecroix:

What is that in src/sage/libs/pari/pari_instance.pyx?

+            #from sage.misc.superseded import deprecation
+            #deprecation(XXXXX, 'passing -1 as PARI variable is deprecated, use None instead')

It's basically a reminder to myself that this really should be deprecated.

The point is that if this ticket gets merged but not the follow-up (because you might stop doing Sage development or anything) we will end up with these two very strange lines.

Could you at least start with a comment

+            # # we should deprecate this because XYZ.
+            #from sage.misc.superseded import deprecation
+            #deprecation(XXXXX, 'passing -1 as PARI variable is deprecated, use None 

Why did you keep so much methods in the gen class in gen.pyx (such as List, Mat, sign, vecmin, vecmax, etc)?

In any case, I didn't want to remove any doctests. I also didn't check very carefully which methods could be removed.

Right. Doctest are indeed important! What is your long term goal with those methods?

Where can I found the implementation and/or documentation of pari_catch_sig_on and co?

See src/sage/libs/pari/pari_err.pxi. But pari_catch_sig_on() and pari_catch_sig_off are just sig_on() and sig_off. This used to be different before we had #14894.

Just for precision: is pari_catch_sig_on just an equivalent to sig_on? If so I really do not understand why there is so much pari_catch_sig_on without any pari_catch_sig_off!

Most of your comments are indeed good comments, but my first priority was to get the general framework (i.e. this ticket) reviewed, we can still improve things later. I prefer to split tickets in smaller pieces, I dislike these huge tickets which do everything.

But I, as a reviewer, do not have your vision of the larger project. I just see one ticket (actually there is also the part 2 #17860).

comment:23 in reply to: ↑ 22 Changed 7 years ago by jdemeyer

Replying to vdelecroix:

Why did you keep so much methods in the gen class in gen.pyx (such as List, Mat, sign, vecmin, vecmax, etc)?

In any case, I didn't want to remove any doctests. I also didn't check very carefully which methods could be removed.

Right. Doctest are indeed important! What is your long term goal with those methods?

In the end, I think it's probably best to store all extra documentation and doctests in a separate file (I haven't thought about the exact mechanism). Then that extra documentation will be put together with the PARI documentation in the auto-generated file.

Just for precision: is pari_catch_sig_on just an equivalent to sig_on?

Yes (although you're recommended to use pari_catch_sig_on in PARI code in case we want to change this).

I really do not understand why there is so much pari_catch_sig_on without any pari_catch_sig_off!

Because the new_gen() method runs pari_catch_sig_off().

comment:24 in reply to: ↑ 21 Changed 7 years ago by jdemeyer

About vecmin and vecmax: those have a pointer argument (a & in the prototype) which is currently not supported.

comment:25 Changed 7 years ago by git

  • Commit changed from a5a6b25ad3c322c03cec85b6c85da4fcb5508cdf to b880274653c46f0f7ee6baa46d6d00a0eec0496a

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

b880274Remove commented deprecation

comment:26 Changed 7 years ago by jdemeyer

  • Status changed from needs_info to needs_review

comment:27 Changed 7 years ago by vdelecroix

  • Status changed from needs_review to positive_review

Let it go and use it!

comment:28 Changed 7 years ago by jdemeyer

Thanks for the review!

comment:29 Changed 7 years ago by vbraun

  • Branch changed from u/jdemeyer/17631 to b880274653c46f0f7ee6baa46d6d00a0eec0496a
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:30 Changed 7 years ago by was

  • Commit b880274653c46f0f7ee6baa46d6d00a0eec0496a deleted

Wow, I'm a little worried about doctests. However, this whole approach is a major step forward! Great work!!

comment:31 Changed 7 years ago by jdemeyer

About the doctests: this ticket is about a very direct interface with upstream PARI. That interface itself is doctested.

The wrapped functions indeed lack doctests, but PARI has quite an extensive test suite (which is run by Sage when installing with SAGE_CHECK=yes).

Note: See TracTickets for help on using tickets.