Opened 3 years ago
Closed 3 years ago
#20241 closed enhancement (fixed)
Separate Sagespecific components from generic Cinterface in PariInstance
Reported by:  defeo  Owned by:  

Priority:  major  Milestone:  sage7.4 
Component:  interfaces  Keywords:  
Cc:  jdemeyer  Merged in:  
Authors:  Luca De Feo  Reviewers:  Jeroen Demeyer 
Report Upstream:  N/A  Work issues:  
Branch:  62f227e (Commits)  Commit:  62f227ebaf34601aa818b60ff6bf2b3d0641a953 
Dependencies:  Stopgaps: 
Description
In preparation for #20238, take out of PariInstance
all components specific to Sage.
Change History (79)
comment:1 Changed 3 years ago by
 Branch set to u/defeo/trac/u/defeo/pari_instance
comment:2 Changed 3 years ago by
 Commit set to 799f2555869086a4a302313545a9b74d8e5597e8
 Dependencies set to #20217
comment:3 followup: ↓ 4 Changed 3 years ago by
Regarding the utility functions: I think primes
belongs in sage.rings.fast_arith
along with prime_range
, while genus2red
should be in sage.schemes.hyperelliptic_curves.HyperellipticCurve_g2_rational_field
which is otherwise completely empty! The other functions don't provide any new functionality (factorials, Chebyshev polynomials, etc.).
comment:4 in reply to: ↑ 3 Changed 3 years ago by
Replying to kedlaya:
Regarding the utility functions: I think
primes
belongs insage.rings.fast_arith
along withprime_range
, whilegenus2red
should be insage.schemes.hyperelliptic_curves.HyperellipticCurve_g2_rational_field
which is otherwise completely empty! The other functions don't provide any new functionality (factorials, Chebyshev polynomials, etc.).
Good points. These would take a lengthy deprecation process, and are better handled after a successful separation of the interface (something we're still having a hard time with).
I think the best place for this comment is in #20238, hence I'm copying it there.
comment:5 Changed 3 years ago by
@kedleya: I don't really understand your comment. This ticket is about the interface between Sage and PARI (which is a purely technical nonmathematical thing), not about how which PARI functions are used where.
comment:6 Changed 3 years ago by
 Branch changed from u/defeo/trac/u/defeo/pari_instance to u/defeo/ticket/20241
comment:7 Changed 3 years ago by
 Commit changed from 799f2555869086a4a302313545a9b74d8e5597e8 to 7ad5c616418f19d84a37311d750b6db0693c4ec2
 Dependencies changed from #20217 to #20217, #21158
Last 10 new commits:
12d9d32  Implement unary operations in interfaces

aae48bd  Merge branch 'ticket/21152' into t/20767/move_coercion_to_element

0a44b08  Remove one doctest for __mul__

54b0fcd  Various minor fixes for the coercion model

2405dde  Merge commit '0a44b082f08c742fbe564c5afdd2f7309fabbb52'; commit '54b0fcdaf2fd69cd64978da526af6774b57a59a8' into t/20767/move_coercion_to_element

e1d2ba4  Move arithmetic using coercion model to Element

b3ea04f  Try reversed operation in richcmp if coercion fails

efc4fd0  Merge branch 't/21163/in_richcmp__fall_back_to_reversed_operation_if_coercion_fails' into t/21158/ticket/21158

71f31a4  Decouple PARI from the Sage coercion model

7ad5c61  Step 1: make the unique PariInstance private

comment:8 Changed 3 years ago by
 Dependencies changed from #20217, #21158 to #21158
 Milestone changed from sage7.2 to sage7.4
 Type changed from PLEASE CHANGE to enhancement
comment:9 Changed 3 years ago by
I don't really know where you are going with this branch, but I thought that we wanted to minimize the use of the global pari
variable?
For example, closure.pyx
should not need it, we just need to make new_gen
and friends functions instead of methods of PariInstance
.
comment:10 Changed 3 years ago by
Yup. Second step. I needed to know who was using it anyway. Any module importing _pari_instance is doing bad, and I'm fixing it. I just pushed this to get help from patchbot.
comment:11 Changed 3 years ago by
 Commit changed from 7ad5c616418f19d84a37311d750b6db0693c4ec2 to 6a4d9bdaff3f7806e3d85f2d5451a46a63b6e130
comment:12 Changed 3 years ago by
Hi @jdemeyer. I'm having trouble compiling this code. gcc fails while compiling complex_double.pyx
, but I cannot figure our the problem. Also matrix_integer_dense.pyx
gives fishy warnings.
Changing slightly L94 in complex_double.pyx
, i.e., using cimport .. as
or similar, it gets even worse: compilation freezes midway, I have to CtrlC it.
I'm giving up for today. Do you have any idea?
comment:13 Changed 3 years ago by
For starters, you cannot declare a function to be cdef inline
in a .pxd
file unless you put the full function body also in the .pxd
file. So this isn't correct:
cdef inline gen new_gen(GEN x)
You have to understand how the compiler works: the compiler for a different module only sees the .pxd
file, so it cannot possibly inline the function new_gen
since it doesn't know what new_gen
does.
It's a bit of a mystery why Cython still accepts this code. GCC gives a error (but sometimes just a warning, maybe it depends on the language) like
[sagelib7.3.rc0] build/cythonized/sage/matrix/matrix_integer_dense.cpp:4828:72: error: ‘__pyx_f_4sage_4libs_4pari_5stack_clear_stack’ declared as an ‘inline’ variable [sagelib7.3.rc0] static CYTHON_INLINE void (*__pyx_f_4sage_4libs_4pari_5stack_clear_stack)(void); /*proto*/
comment:14 Changed 3 years ago by
Note that it is perfectly legal to have an inline function in a .pyx
file and still export it in the .pxd
file. Just don't put the inline
in the .pxd
. This way, the function can be inlined in the .pyx
file (like the call from new_gen
to new_gen_noclear
) but other modules will do a noninline function call.
comment:15 Changed 3 years ago by
 Branch changed from u/defeo/ticket/20241 to u/jdemeyer/ticket/20241
comment:16 Changed 3 years ago by
 Commit changed from 6a4d9bdaff3f7806e3d85f2d5451a46a63b6e130 to 3740c06e738970213afc1af1dfe3457f8a8a3f60
New commits:
3740c06  Fix inlining in sage.libs.pari.stack

comment:17 Changed 3 years ago by
Sage at least compiles now. It doesn't actually start, but I'll leave it to you to continue working on this.
comment:18 Changed 3 years ago by
Makes perfect sense. What I don't get is why you can put cdef inline
in METHOD definitions in a .pxd
. I just cutpasted that line from the definition of PariInstance
, and it had been actually compiling, booting, and passing tests (hadn't tried many tests, though) for quite some time before this... that's why I hate Cython!
Anyway, thanks for your help. I'll keep chasing the breakages tomorrow.
comment:19 Changed 3 years ago by
 Branch changed from u/jdemeyer/ticket/20241 to u/defeo/ticket/20241
comment:20 followups: ↓ 21 ↓ 22 Changed 3 years ago by
 Commit changed from 3740c06e738970213afc1af1dfe3457f8a8a3f60 to 341b49b925dea6aa815b42abec0cfd9d4ffd4da4
Ok, fixed the startup bug, and tests (mostly) pass. Next step is to move all those conversion functions out of the way.
I'm scratching my head about allocatemem
and companions, though. allocatemem
is a gp function, but it's tied to gen
by auto_gen.pxi
, so that you can do
sage: size = pari('10^8') sage: size.allocatemem()
which feels very bad. Wouldn't it make sense to blacklist this one?
On the other hand there is a manually coded allocatemem
method in PariInstance
which gives a much more sensible
sage: pari.allocatemem(10^8)
and also has an additional silent
parameter to behave more like gp. There are also methods stacksize
and stacksizemax
to read the stack sizes. It seems more logical to put these three functions in stack.pyx
, but if we want the user to call them from the command line, it is better to leave methods in PariInstance
.
Question: given the current handling of PARI's stack, do we still want the user to easily call these functions?
New commits:
341b49b  Step 2: Move new_gen and related functions out of PariIinstance.

comment:21 in reply to: ↑ 20 Changed 3 years ago by
comment:22 in reply to: ↑ 20 Changed 3 years ago by
Replying to defeo:
Question: given the current handling of PARI's stack, do we still want the user to easily call these functions?
I don't see the problem. They work perfectly fine and make sense. Unless they are problematic for the implementation, I would just keep them as they are. Another reason is that we should focus on finishing this deliverable. We can always clean up such details afterwards.
comment:23 Changed 3 years ago by
 Commit changed from 341b49b925dea6aa815b42abec0cfd9d4ffd4da4 to a57e6c37a7147426e8e3533bbcb736c6795daab2
comment:24 Changed 3 years ago by
 Commit changed from a57e6c37a7147426e8e3533bbcb736c6795daab2 to d23169ad79e100a44997c64492539961efd1c45c
Branch pushed to git repo; I updated commit sha1. New commits:
d23169a  Moved Sagespecific Pari conversion functions to separate file.

comment:25 followups: ↓ 26 ↓ 27 Changed 3 years ago by
Ok, so I managed to move all Sagespecific conversion functions from PariInstance
to a separate file. We're getting closer to the goal: the only imports left in pari_instance.p??
from the sage library are
from sage.ext.memory import init_memory_functions from sage.misc.superseded import deprecation, deprecated_function_alias
There's a couple of conversion functions left in PariInstance
which are not specific to Sage:
cdef gen new_gen_from_int(self, int value) cdef gen new_t_POL_from_int_star(self, int *vals, int length, long varnum) cdef gen double_to_gen_c(self, double) cdef GEN double_to_GEN(self, double)
plus a python method PariInstance.double_to_gen
. Should we put these in convert.pyx
, or maybe another file?
Note that new_gen_from_int
is only called once in padic_capped_relative_element.pyx
, and just to create 0.
It is also not very clear to me what the purpose of double_to_gen
and double_to_gen_c
is. double_to_gen_c
is called only in line 1585 of real_double.pyx
, and that's in a def
ed function. Is this really faster than calling double_to_gen
?
And what's the meaning of this, anyway:
sage: pari(0.1) == pari.double_to_gen(0.1) False sage: pari(0.1) == pari(0.1) True sage: pari.double_to_gen(0.1) == pari.double_to_gen(0.1) True sage: pari(0.1)  pari.double_to_gen(0.1) 5.54975987041018 E18 sage: pari(0.1)  pari(0.1) 0.E20 sage: pari.double_to_gen(0.1)  pari.double_to_gen(0.1) 0.E20
New commits:
d23169a  Moved Sagespecific Pari conversion functions to separate file.

comment:26 in reply to: ↑ 25 Changed 3 years ago by
Replying to defeo:
Ok, so I managed to move all Sagespecific conversion functions from
PariInstance
to a separate file. We're getting closer to the goal: the only imports left inpari_instance.p??
from the sage library arefrom sage.ext.memory import init_memory_functions from sage.misc.superseded import deprecation, deprecated_function_aliasThere's a couple of conversion functions left in
PariInstance
which are not specific to Sage:cdef gen new_gen_from_int(self, int value) cdef gen new_t_POL_from_int_star(self, int *vals, int length, long varnum) cdef gen double_to_gen_c(self, double) cdef GEN double_to_GEN(self, double)plus a python method
PariInstance.double_to_gen
. Should we put these inconvert.pyx
Fine for me.
Note that
new_gen_from_int
is only called once inpadic_capped_relative_element.pyx
, and just to create 0.It is also not very clear to me what the purpose of
double_to_gen
anddouble_to_gen_c
is.
double_to_gen
seems unused and untested. I do not mind removing it.
double_to_gen_c
is called only in line 1585 ofreal_double.pyx
, and that's in adef
ed function. Is this really faster than callingdouble_to_gen
?
Obviously yes. Calling a cdef
function is much faster than calling a def
function. It does not matter where it is called from. But I would suggest renaming double_to_gen_c
to double_to_gen
.
comment:27 in reply to: ↑ 25 Changed 3 years ago by
Replying to defeo:
sage: pari(0.1) == pari.double_to_gen(0.1) False
That's a "feature" because of RealLiteral
:
sage: x = RR(0.1); y = RR(float(0.1)) sage: x == y True sage: parent(x) is parent(y) True sage: RR64 = RealField(64) sage: RR64(x) == RR64(y) False sage: pari(x) == pari(y) False
comment:28 Changed 3 years ago by
 Commit changed from d23169ad79e100a44997c64492539961efd1c45c to c7b5d6d03ed6213a67dde4f8947e2d7f8bbb9246
Branch pushed to git repo; I updated commit sha1. New commits:
c7b5d6d  Moved more conversion functions to convert.pyx

comment:29 Changed 3 years ago by
 Commit changed from c7b5d6d03ed6213a67dde4f8947e2d7f8bbb9246 to a8b8b7c5bfd6f412eb0f35e5009bc888a80e83b2
Branch pushed to git repo; I updated commit sha1. New commits:
a8b8b7c  Fixed minor bugs, changed behaviour of `pari(complex(x,y))`

comment:30 Changed 3 years ago by
You might consider a separate ticket for moving the conversion functions.
comment:31 Changed 3 years ago by
 Branch changed from u/defeo/ticket/20241 to u/jdemeyer/ticket/20241
comment:32 Changed 3 years ago by
 Branch changed from u/jdemeyer/ticket/20241 to u/defeo/ticket/20241
Why did you remove pari.stacksize()
and similar? I would keep them as methods of pari
.
comment:33 Changed 3 years ago by
 Branch changed from u/defeo/ticket/20241 to u/jdemeyer/ticket/20241
comment:34 Changed 3 years ago by
 Commit changed from a8b8b7c5bfd6f412eb0f35e5009bc888a80e83b2 to eaa4be48d560a36c3f205381e742052d82db10b6
I don't really see the point of this new init()
function. What's wrong with using the __init__
method for this?
New commits:
eaa4be4  Merge tag '7.4.beta3' into t/20241/ticket/20241

comment:35 Changed 3 years ago by
 Branch changed from u/jdemeyer/ticket/20241 to u/defeo/ticket/20241
comment:36 Changed 3 years ago by
 Commit changed from eaa4be48d560a36c3f205381e742052d82db10b6 to 69adf8c133e5b6a3871335b2fb47ad08cfb32ae2
 Status changed from new to needs_review
Agreed to revert to a normal constructor, no init()
factory method.
New commits:
66f7ca6  Updated docstrings and copyright headers

a252743  Removed reduntant new_gen_from_int function

d282135  Reverted to constructor for PariInstance, no factory method

7852f1c  Cosmetic change

69adf8c  Fixed docstring for deprecated method

comment:37 Changed 3 years ago by
 Branch changed from u/defeo/ticket/20241 to u/jdemeyer/ticket/20241
comment:38 Changed 3 years ago by
 Commit changed from 69adf8c133e5b6a3871335b2fb47ad08cfb32ae2 to 3151ec4685b19979f21db9aa17afbc472f962d7f
New commits:
7bfd42a  Merge tag '7.4.beta2' into t/20767/move_coercion_to_element

343131a  Fix doctest for Trac #20767

a501048  Move category attribute lookup to CategoryObject.getattr_from_category()

0fabb7a  Optimize CategoryObject.getattr_from_category()

74041f3  Mark broken testsuites as # known bug

83cca0f  Merge commit '343131a9951dc842c462809b9366b13416382dfc'; commit '91c436752ba82d512efdf67e2890d5ecbe0b7234'; commit '74041f30d8de4c15055345f14340a4f443236ffa' into HEAD

3151ec4  Separate Sagespecific components from generic Cinterface in PariInstance

comment:39 Changed 3 years ago by
 Commit changed from 3151ec4685b19979f21db9aa17afbc472f962d7f to f35e562311b954e66516d3e111311b4d573480a5
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
f35e562  Separate Sagespecific components from generic Cinterface in PariInstance

comment:40 Changed 3 years ago by
 Commit changed from f35e562311b954e66516d3e111311b4d573480a5 to e363e4c6b7a4b9d5c71cbc37e4bf4e848f881899
Branch pushed to git repo; I updated commit sha1. New commits:
e363e4c  Simplify module_list

comment:41 Changed 3 years ago by
 Commit changed from e363e4c6b7a4b9d5c71cbc37e4bf4e848f881899 to 968519c0b552d8e94262b58580fe2003619bd515
Branch pushed to git repo; I updated commit sha1. New commits:
968519c  Cleanup imports; add __future__ statements

comment:42 Changed 3 years ago by
 Branch changed from u/jdemeyer/ticket/20241 to u/defeo/ticket/20241
comment:43 Changed 3 years ago by
 Commit changed from 968519c0b552d8e94262b58580fe2003619bd515 to e88d56cdc1500da91142c11191b486fd1666a787
New commits:
e88d56c  Fixed broken doctest in autogenerationy

comment:44 Changed 3 years ago by
Just a reminder: I plan to review this when all dependencies have been merged. Unfortunately, this is currently blocked by #21441.
comment:45 Changed 3 years ago by
 Branch changed from u/defeo/ticket/20241 to u/jdemeyer/ticket/20241
comment:46 Changed 3 years ago by
 Commit changed from e88d56cdc1500da91142c11191b486fd1666a787 to 57e1cbaf2c5a4d43d3796037577696a709a1138d
comment:47 Changed 3 years ago by
 Dependencies #21158 deleted
 Reviewers set to Jeroen Demeyer
comment:48 Changed 3 years ago by
 Commit changed from 57e1cbaf2c5a4d43d3796037577696a709a1138d to 6ef8de2e479449507cf2fddcc67c3b0733820fd9
Branch pushed to git repo; I updated commit sha1. New commits:
6ef8de2  Cosmetic changes to sage.libs.pari.convert

comment:49 followup: ↓ 53 Changed 3 years ago by
I am wondering what to do with convert_sage
. First of all, it contains conversions both to/from GMP as well as conversions to/from FLINT. These should be split up in two different modules (I will do that).
But a bigger question is: should these become part of Sage or part of CyPari?? Those conversion routines are not directly related to Sage, but to GMP/FLINT which may be of interest to other libraries too.
comment:50 Changed 3 years ago by
 Commit changed from 6ef8de2e479449507cf2fddcc67c3b0733820fd9 to fb54db3ed52b3718cb9aef8eec29101c5ce5dadd
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
fb54db3  Cosmetic changes to sage.libs.pari.convert

comment:51 Changed 3 years ago by
 Commit changed from fb54db3ed52b3718cb9aef8eec29101c5ce5dadd to 686d2997e355918a4b73608ebd87638391eb4580
Branch pushed to git repo; I updated commit sha1. New commits:
686d299  Split convert_sage in convert_flint and convert_gmp

comment:52 Changed 3 years ago by
 Commit changed from 686d2997e355918a4b73608ebd87638391eb4580 to 54117801c99e58bf3ab0e981d3719ea11704fa4c
Branch pushed to git repo; I updated commit sha1. New commits:
5411780  Undo moving of imports in gentoobj()

comment:53 in reply to: ↑ 49 ; followup: ↓ 55 Changed 3 years ago by
Replying to jdemeyer:
I am wondering what to do with
convert_sage
. First of all, it contains conversions both to/from GMP as well as conversions to/from FLINT. These should be split up in two different modules (I will do that).
Good. Thx.
But a bigger question is: should these become part of Sage or part of CyPari?? Those conversion routines are not directly related to Sage, but to GMP/FLINT which may be of interest to other libraries too.
If they where in CyPari?, then CyPari? would need GMP/Flint in order to compile, wouldn't it?
comment:54 Changed 3 years ago by
 Commit changed from 54117801c99e58bf3ab0e981d3719ea11704fa4c to d522155ff2f79eed03f73a637d8164cff9cf6f6c
comment:55 in reply to: ↑ 53 Changed 3 years ago by
comment:56 Changed 3 years ago by
Also, cysignals
currently has an optional dependency on PARI. It seems to work fine.
comment:57 Changed 3 years ago by
For me, this ticket is good to merge. Luca, can you look at the commits that I added (each of them is relatively small and selfcontained)?
comment:58 Changed 3 years ago by
I think commit d522155ff2f79eed03f73a637d8164cff9cf6f6c is a little brittle.
If I understand autogeneration correctly, this doesn't break because no method of PariInstance_auto
has a PariArgumentSeriesPrec
argument (prototype P
). I don't know if it is safe to assume that PARI will never have such a function.
comment:59 followup: ↓ 61 Changed 3 years ago by
Also, I'm not completely happy about 4d2ad6a97a3c0750768dfa770c113a7cd30c29de. I feel it's bad cimporting pari_instace
in padic_capped_relative_element.pyx
.
In my opinion, external software should use the "public" pari
API whenever possible, unless it has good reasons to fiddle with the internals of CyPari?. Is there any good reason here?
comment:60 followup: ↓ 69 Changed 3 years ago by
Concerning convert_gmp.pyx
, docstrings and function names are a bit schizophrenic on whether these are converstion to/from GMP or MPIR. Presumably they work with both?
comment:61 in reply to: ↑ 59 ; followup: ↓ 62 Changed 3 years ago by
Replying to defeo:
In my opinion, external software should use the "public"
pari
API whenever possible
I consider this as part of the public API. Why do you consider the Python API public but the Cython API private?
comment:62 in reply to: ↑ 61 ; followup: ↓ 63 Changed 3 years ago by
Replying to jdemeyer:
Replying to defeo:
In my opinion, external software should use the "public"
pari
API whenever possibleI consider this as part of the public API. Why do you consider the Python API public but the Cython API private?
It's not exactly that. I consider the Cython API also public (unless there are underscores). But when there is duplication of functionality (e.g., any def
ed or cpdef
ed method of pari_instance
), I give priority to the Python API.
I mean, if all you want is to get PARI's zero, just do like any common mortal and call pari.zero()
. Shorter, more readable, easier to understand.
Of course, it's not a big deal.
comment:63 in reply to: ↑ 62 ; followup: ↓ 64 Changed 3 years ago by
Replying to defeo:
I mean, if all you want is to get PARI's zero, just do like any common mortal and call
pari.zero()
. Shorter, more readable, easier to understand.
...but slower. I don't think it matters in this specific case, but we really want users to call pari_instance.zero()
using Cython if speed is important.
comment:64 in reply to: ↑ 63 ; followup: ↓ 65 Changed 3 years ago by
...but slower.
By how much?
I don't think it matters in this specific case, but we really want users to call
pari_instance.zero()
using Cython if speed is important.
That's why there is no underscore in front of pari_instance
. But, as you say, speed is not important here.
comment:65 in reply to: ↑ 64 ; followup: ↓ 66 Changed 3 years ago by
Replying to defeo:
...but slower.
By how much?
I haven't profiled, but I would guess at least an order of magnitude. Python methods are slow.
But that doesn't matter, the point is that pari_instance
and the zero()
method are part of the public Cython API.
comment:66 in reply to: ↑ 65 ; followup: ↓ 68 Changed 3 years ago by
By how much?
I haven't profiled, but I would guess at least an order of magnitude. Python methods are slow.
I have. The overhead is ~50ns. pari_instance.zero()
itself takes ~50ns.
But that doesn't matter,
As you say. Who cares about 50ns? So we agree that speed is not an argument here. In my opinion readability is, instead.
the point is that
pari_instance
and thezero()
method are part of the public Cython API.
I already replied to this point saying that pari.zero()
is "more public". You haven't given an explanation of why my point is not good, other than pari_instance.zero()
is "public enough".
I do not want to hold this ticket on this very minor detail. If we cannot convince each other, commit minimality wins, so the code stays as it is now.
My other two comments are more serious, though.
comment:67 followup: ↓ 72 Changed 3 years ago by
 Commit changed from d522155ff2f79eed03f73a637d8164cff9cf6f6c to 6f815cd0dcba998a748b59500d3820ff2efc9716
Branch pushed to git repo; I updated commit sha1. New commits:
6f815cd  Put back convert_code for PariInstanceArgument

comment:68 in reply to: ↑ 66 ; followup: ↓ 73 Changed 3 years ago by
Replying to defeo:
I already replied to this point saying that
pari.zero()
is "more public". You haven't given an explanation of why my point is not good, other thanpari_instance.zero()
is "public enough".
Could you be convinced by
from ... cimport pari_instance as pari
and then using pari.zero()
? IMHO, that would be equally readable as the original code while still being fast.
comment:69 in reply to: ↑ 60 Changed 3 years ago by
Replying to defeo:
Concerning
convert_gmp.pyx
, docstrings and function names are a bit schizophrenic on whether these are converstion to/from GMP or MPIR. Presumably they work with both?
I totally agree that function names are really inconsistent. But I would keep the names for now, we can make a new ticket to change the names. I'll have a look at the docstrings.
comment:70 Changed 3 years ago by
 Commit changed from 6f815cd0dcba998a748b59500d3820ff2efc9716 to 8b3013e1c2b698a5cd6bedef03bf90cc29cbb29d
comment:71 Changed 3 years ago by
I think this takes care of your other comments. So that leaves the pari_instance.zero()
issue. What do you think of 68
comment:72 in reply to: ↑ 67 ; followup: ↓ 74 Changed 3 years ago by
Replying to git:
Branch pushed to git repo; I updated commit sha1. New commits:
6f815cd Put back convert_code for PariInstanceArgument
Ok, this addresses one of my concerns. But I'm curious: what's the rationale behind writing
def ...(self, ...): cdef PariInstance pari_instance = <PariInstance>self
instead of simply writing
def ...(pari_instance, ...):
comment:73 in reply to: ↑ 68 Changed 3 years ago by
Replying to jdemeyer:
Replying to defeo:
I already replied to this point saying that
pari.zero()
is "more public". You haven't given an explanation of why my point is not good, other thanpari_instance.zero()
is "public enough".Could you be convinced by
from ... cimport pari_instance as pariand then using
pari.zero()
? IMHO, that would be equally readable as the original code while still being fast.
So it seems you care about speed, here? I thought you didn't. Why don't you retrun pari_instance.PARI_ZERO
, then? I personally don't see how speed can matter here: it's a seldom executed ifbranch in a function that is probably orders of magnitude slower than the pythoncall overhead.
I agree that's more readable, though. I'd prefer that to the current situation.
comment:74 in reply to: ↑ 72 Changed 3 years ago by
Replying to defeo:
what's the rationale behind writing
def ...(self, ...): cdef PariInstance pari_instance = <PariInstance>selfinstead of simply writing
def ...(pari_instance, ...):
The only reason is the usual convention that the "self" argument of a method should be called self
.
And ideally, maybe we can get rid of the pari_instance.get_series_precision()
call to avoid the pari_instance
argument completely.
comment:75 Changed 3 years ago by
 Commit changed from 8b3013e1c2b698a5cd6bedef03bf90cc29cbb29d to 62f227ebaf34601aa818b60ff6bf2b3d0641a953
Branch pushed to git repo; I updated commit sha1. New commits:
62f227e  cimport pari_instance as pari

comment:76 Changed 3 years ago by
 Status changed from needs_review to positive_review
Sorry for the delay, good to go for me.
comment:77 Changed 3 years ago by
Thanks! Could you also have a look at #21703 please?
comment:78 Changed 3 years ago by
yup. looking at it.
comment:79 Changed 3 years ago by
 Branch changed from u/jdemeyer/ticket/20241 to 62f227ebaf34601aa818b60ff6bf2b3d0641a953
 Resolution set to fixed
 Status changed from positive_review to closed
I'm pushing a failed attempt at splitting
PariInstance
into a genericPariInstance
cython type, and a SagespecificSagePariInstance
.This is problematic because of the global
sage.libs.pari.pari_instance.pari
variable: it is not clear where this variable should go.Some concrete suggestions:
PariInstance
serves three main purposes:primes
,genus2_reduction
, ...)Of these, only the first one really needs a Cython type. I suggest:
PariStack
around the pari stack. This should not be handled as a global variable, but rather as a singleton class (we cannot have more than one pari stack, because of global variables).PariInstance
hold a pointer to aPariStack
. Havegen
's hold a pointer to aPariInstance
. (Maybe this double wrapping is superfluous?)I don't know quite well what to do with utility functions such as
primes
. They may stay as methods ofPariInstance
, or maybe go to a different namespace as pure functions (they should take aPariInstance
orPariStack
as parameter, then).Last 10 new commits:
Move memory functions to cysignals
Rename sage_malloc > sig_malloc and friends
Get rid of factorint_withproof_sage in PARI interface
Stop using deprecated PARI factoring features
Merge branch 't/20205/get_rid_of_factorint_withproof_sage_in_pari_interface' into HEAD
Replace pari_catch_sig_on by sig_on
Deprecate PARI nth_prime and prime_list
Remove redundant functions from pari_instance.pyx
Failed attempt to split PariInstance
Split pxd file too