#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: |
Description (last modified by )
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
- Dependencies set to #16997
comment:2 Changed 7 years ago by
- Cc vdelecroix added
comment:3 Changed 7 years ago by
- 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
- Commit set to a18d97fbe8c4a469c470eac06c143fc2a2deca41
comment:5 Changed 7 years ago by
- Description modified (diff)
- Summary changed from Auto-generate gen.pyx to Auto-generate gen.pyx -- part 1
comment:6 Changed 7 years ago by
- Commit changed from a18d97fbe8c4a469c470eac06c143fc2a2deca41 to 58af466e7c33917b3fdcbf45449a918a1f0df761
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
58af466 | Auto-generate parts of gen.pyx and pari_instance.pyx
|
comment:7 Changed 7 years ago by
- Commit changed from 58af466e7c33917b3fdcbf45449a918a1f0df761 to fd9074ffb0b8ddd94170bdb0ff652b0764dd9a81
comment:8 Changed 7 years ago by
- Status changed from new to needs_review
comment:9 follow-ups: ↓ 13 ↓ 16 Changed 7 years ago by
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 makeread_pari_desc
an iterator and avoid thesorted
inPariFunctionGenerator.__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 ownpari
. 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
Where are the review commits?
comment:11 Changed 7 years ago by
- Branch changed from u/jdemeyer/ticket/17631 to u/uvdelecroix/17631
- Commit fd9074ffb0b8ddd94170bdb0ff652b0764dd9a81 deleted
here they are
comment:12 Changed 7 years ago by
- Branch changed from u/uvdelecroix/17631 to u/vdelecroix/17631
- Commit set to 51236663e6488db63506102850bc69628a16764a
New commits:
ade4f75 | Upgrade PARI to git master
|
fd9074f | Auto-generate parts of gen.pyx and pari_instance.pyx
|
82a67fc | trac #17631 (reviewer commit 1): add a force argument
|
8da123a | trac #17631 (reviewer commit 2): pari_src function
|
eb5a737 | trac #17631 (reviewer commit 3): make read_pari_desc an iterator
|
5123666 | trac #17631 (reviewer commit 3): simpler read_decl
|
comment:13 in reply to: ↑ 9 Changed 7 years ago by
Replying to vdelecroix:
- Don't we want to keep the order of the
pari.desc
file? It would be enough to makeread_pari_desc
an iterator and avoid thesorted
inPariFunctionGenerator.__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
- Branch changed from u/vdelecroix/17631 to u/jdemeyer/17631
comment:15 Changed 7 years ago by
- Commit changed from 51236663e6488db63506102850bc69628a16764a to 98022fd94d8483a4b7f2b19af598bc8c1d3eb1dc
- Reviewers set to Vincent Delecroix
comment:16 in reply to: ↑ 9 Changed 7 years ago by
Replying to vdelecroix:
Is it on purpose that
decl.pxi
anddeclinl.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 inparisage.h
still true#undef coeff /* Conflicts with NTL */
Yes, we should leave that.
comment:17 Changed 7 years ago by
- Description modified (diff)
comment:18 Changed 7 years ago by
- Commit changed from 98022fd94d8483a4b7f2b19af598bc8c1d3eb1dc to a5a6b25ad3c322c03cec85b6c85da4fcb5508cdf
Branch pushed to git repo; I updated commit sha1. New commits:
a5a6b25 | Merge tag '6.6.beta4' into t/17631/17631
|
comment:19 follow-up: ↓ 21 Changed 7 years ago by
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
- Status changed from needs_review to needs_info
comment:21 in reply to: ↑ 19 ; follow-ups: ↓ 22 ↓ 24 Changed 7 years ago by
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 ingen.pyx
(such asList
,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: ↓ 23 Changed 7 years ago by
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 ingen.pyx
(such asList
,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
. Butpari_catch_sig_on()
andpari_catch_sig_off
are justsig_on()
andsig_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
Replying to vdelecroix:
Why did you keep so much methods in the
gen
class ingen.pyx
(such asList
,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 tosig_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 anypari_catch_sig_off
!
Because the new_gen()
method runs pari_catch_sig_off()
.
comment:24 in reply to: ↑ 21 Changed 7 years ago by
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
- Commit changed from a5a6b25ad3c322c03cec85b6c85da4fcb5508cdf to b880274653c46f0f7ee6baa46d6d00a0eec0496a
Branch pushed to git repo; I updated commit sha1. New commits:
b880274 | Remove commented deprecation
|
comment:26 Changed 7 years ago by
- Status changed from needs_info to needs_review
comment:27 Changed 7 years ago by
- Status changed from needs_review to positive_review
Let it go and use it!
comment:28 Changed 7 years ago by
Thanks for the review!
comment:29 Changed 7 years ago by
- 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
- 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
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
).
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
Auto-generate parts of gen.pyx and pari_instance.pyx