Opened 3 years ago

Closed 3 years ago

#20241 closed enhancement (fixed)

Separate Sage-specific components from generic C-interface in PariInstance

Reported by: defeo Owned by:
Priority: major Milestone: sage-7.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 defeo

  • Branch set to u/defeo/trac/u/defeo/pari_instance

comment:2 Changed 3 years ago by defeo

  • Commit set to 799f2555869086a4a302313545a9b74d8e5597e8
  • Dependencies set to #20217

I'm pushing a failed attempt at splitting PariInstance into a generic PariInstance cython type, and a Sage-specific SagePariInstance.

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:

  • As a wrapper around the pari stack,
  • As a collection of conversion methods to and from pari types,
  • As a namespace for a handful of functions, for lack of a better place (e.g. primes, genus2_reduction, ...)

Of these, only the first one really needs a Cython type. I suggest:

  • To make a proper abstraction 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).
  • Move conversion functions to a separate namespace, as #20226 started doing.
  • Have PariInstance hold a pointer to a PariStack. Have gen's hold a pointer to a PariInstance. (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 of PariInstance, or maybe go to a different namespace as pure functions (they should take a PariInstance or PariStack as parameter, then).


Last 10 new commits:

dce67fcMove memory functions to cysignals
4bb8337Rename sage_malloc -> sig_malloc and friends
5ab73c1Get rid of factorint_withproof_sage in PARI interface
c0ed97aStop using deprecated PARI factoring features
edc5ce2Merge branch 't/20205/get_rid_of_factorint_withproof_sage_in_pari_interface' into HEAD
5fb408dReplace pari_catch_sig_on by sig_on
d5c934cDeprecate PARI nth_prime and prime_list
d7d2d7dRemove redundant functions from pari_instance.pyx
282b187Failed attempt to split PariInstance
799f255Split pxd file too

comment:3 follow-up: Changed 3 years ago by kedlaya

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 defeo

Replying to kedlaya:

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.).

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 jdemeyer

@kedleya: I don't really understand your comment. This ticket is about the interface between Sage and PARI (which is a purely technical non-mathematical thing), not about how which PARI functions are used where.

comment:6 Changed 3 years ago by defeo

  • Branch changed from u/defeo/trac/u/defeo/pari_instance to u/defeo/ticket/20241

comment:7 Changed 3 years ago by defeo

  • Commit changed from 799f2555869086a4a302313545a9b74d8e5597e8 to 7ad5c616418f19d84a37311d750b6db0693c4ec2
  • Dependencies changed from #20217 to #20217, #21158

Last 10 new commits:

12d9d32Implement unary operations in interfaces
aae48bdMerge branch 'ticket/21152' into t/20767/move_coercion_to_element
0a44b08Remove one doctest for __mul__
54b0fcdVarious minor fixes for the coercion model
2405ddeMerge commit '0a44b082f08c742fbe564c5afdd2f7309fabbb52'; commit '54b0fcdaf2fd69cd64978da526af6774b57a59a8' into t/20767/move_coercion_to_element
e1d2ba4Move arithmetic using coercion model to Element
b3ea04fTry reversed operation in richcmp if coercion fails
efc4fd0Merge branch 't/21163/in_richcmp__fall_back_to_reversed_operation_if_coercion_fails' into t/21158/ticket/21158
71f31a4Decouple PARI from the Sage coercion model
7ad5c61Step 1: make the unique PariInstance private

comment:8 Changed 3 years ago by jdemeyer

  • Dependencies changed from #20217, #21158 to #21158
  • Milestone changed from sage-7.2 to sage-7.4
  • Type changed from PLEASE CHANGE to enhancement

comment:9 Changed 3 years ago by jdemeyer

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 defeo

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 git

  • Commit changed from 7ad5c616418f19d84a37311d750b6db0693c4ec2 to 6a4d9bdaff3f7806e3d85f2d5451a46a63b6e130

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

91c4367Remove useless "return False" in SR._coerce_map_from_()
2c1adfaMerge branch 't/21158/ticket/21158' into t/20241/ticket/20241
6a4d9bdTaking new_gen and friends out of PariInstance. Does not compile.

comment:12 Changed 3 years ago by defeo

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 Ctrl-C it.

I'm giving up for today. Do you have any idea?

comment:13 Changed 3 years ago by jdemeyer

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

[sagelib-7.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
[sagelib-7.3.rc0]  static CYTHON_INLINE void (*__pyx_f_4sage_4libs_4pari_5stack_clear_stack)(void); /*proto*/
Last edited 3 years ago by jdemeyer (previous) (diff)

comment:14 Changed 3 years ago by jdemeyer

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 non-inline function call.

comment:15 Changed 3 years ago by jdemeyer

  • Branch changed from u/defeo/ticket/20241 to u/jdemeyer/ticket/20241

comment:16 Changed 3 years ago by jdemeyer

  • Commit changed from 6a4d9bdaff3f7806e3d85f2d5451a46a63b6e130 to 3740c06e738970213afc1af1dfe3457f8a8a3f60

New commits:

3740c06Fix inlining in sage.libs.pari.stack

comment:17 Changed 3 years ago by jdemeyer

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 defeo

Makes perfect sense. What I don't get is why you can put cdef inline in METHOD definitions in a .pxd. I just cut-pasted 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 defeo

  • Branch changed from u/jdemeyer/ticket/20241 to u/defeo/ticket/20241

comment:20 follow-ups: Changed 3 years ago by defeo

  • 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:

341b49bStep 2: Move new_gen and related functions out of PariIinstance.

comment:21 in reply to: ↑ 20 Changed 3 years ago by jdemeyer

Replying to defeo:

Wouldn't it make sense to blacklist this one?

Sure.

comment:22 in reply to: ↑ 20 Changed 3 years ago by jdemeyer

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 git

  • Commit changed from 341b49b925dea6aa815b42abec0cfd9d4ffd4da4 to a57e6c37a7147426e8e3533bbcb736c6795daab2

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

decf7acFixed doctest in auto generation
a57e6c3Reverted modifications to Python stack management methods

comment:24 Changed 3 years ago by git

  • Commit changed from a57e6c37a7147426e8e3533bbcb736c6795daab2 to d23169ad79e100a44997c64492539961efd1c45c

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

d23169aMoved Sage-specific Pari conversion functions to separate file.

comment:25 follow-ups: Changed 3 years ago by defeo

Ok, so I managed to move all Sage-specific 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 defed 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 E-18
sage: pari(0.1) - pari(0.1)
0.E-20
sage: pari.double_to_gen(0.1) - pari.double_to_gen(0.1)
0.E-20

New commits:

d23169aMoved Sage-specific Pari conversion functions to separate file.

comment:26 in reply to: ↑ 25 Changed 3 years ago by jdemeyer

Replying to defeo:

Ok, so I managed to move all Sage-specific 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

Fine for me.

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 seems unused and untested. I do not mind removing it.

double_to_gen_c is called only in line 1585 of real_double.pyx, and that's in a defed function. Is this really faster than calling double_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 jdemeyer

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 git

  • Commit changed from d23169ad79e100a44997c64492539961efd1c45c to c7b5d6d03ed6213a67dde4f8947e2d7f8bbb9246

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

c7b5d6dMoved more conversion functions to convert.pyx

comment:29 Changed 3 years ago by git

  • Commit changed from c7b5d6d03ed6213a67dde4f8947e2d7f8bbb9246 to a8b8b7c5bfd6f412eb0f35e5009bc888a80e83b2

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

a8b8b7cFixed minor bugs, changed behaviour of `pari(complex(x,y))`

comment:30 Changed 3 years ago by jdemeyer

You might consider a separate ticket for moving the conversion functions.

comment:31 Changed 3 years ago by jdemeyer

  • Branch changed from u/defeo/ticket/20241 to u/jdemeyer/ticket/20241

comment:32 Changed 3 years ago by jdemeyer

  • 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 jdemeyer

  • Branch changed from u/defeo/ticket/20241 to u/jdemeyer/ticket/20241

comment:34 Changed 3 years ago by jdemeyer

  • 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:

eaa4be4Merge tag '7.4.beta3' into t/20241/ticket/20241

comment:35 Changed 3 years ago by defeo

  • Branch changed from u/jdemeyer/ticket/20241 to u/defeo/ticket/20241

comment:36 Changed 3 years ago by defeo

  • 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:

66f7ca6Updated docstrings and copyright headers
a252743Removed reduntant new_gen_from_int function
d282135Reverted to constructor for PariInstance, no factory method
7852f1cCosmetic change
69adf8cFixed docstring for deprecated method

comment:37 Changed 3 years ago by jdemeyer

  • Branch changed from u/defeo/ticket/20241 to u/jdemeyer/ticket/20241

comment:38 Changed 3 years ago by jdemeyer

  • Commit changed from 69adf8c133e5b6a3871335b2fb47ad08cfb32ae2 to 3151ec4685b19979f21db9aa17afbc472f962d7f

New commits:

7bfd42aMerge tag '7.4.beta2' into t/20767/move_coercion_to_element
343131aFix doctest for Trac #20767
a501048Move category attribute lookup to CategoryObject.getattr_from_category()
0fabb7aOptimize CategoryObject.getattr_from_category()
74041f3Mark broken testsuites as # known bug
83cca0fMerge commit '343131a9951dc842c462809b9366b13416382dfc'; commit '91c436752ba82d512efdf67e2890d5ecbe0b7234'; commit '74041f30d8de4c15055345f14340a4f443236ffa' into HEAD
3151ec4Separate Sage-specific components from generic C-interface in PariInstance

comment:39 Changed 3 years ago by git

  • Commit changed from 3151ec4685b19979f21db9aa17afbc472f962d7f to f35e562311b954e66516d3e111311b4d573480a5

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

f35e562Separate Sage-specific components from generic C-interface in PariInstance

comment:40 Changed 3 years ago by git

  • Commit changed from f35e562311b954e66516d3e111311b4d573480a5 to e363e4c6b7a4b9d5c71cbc37e4bf4e848f881899

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

e363e4cSimplify module_list

comment:41 Changed 3 years ago by git

  • Commit changed from e363e4c6b7a4b9d5c71cbc37e4bf4e848f881899 to 968519c0b552d8e94262b58580fe2003619bd515

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

968519cCleanup imports; add __future__ statements

comment:42 Changed 3 years ago by defeo

  • Branch changed from u/jdemeyer/ticket/20241 to u/defeo/ticket/20241

comment:43 Changed 3 years ago by jdemeyer

  • Commit changed from 968519c0b552d8e94262b58580fe2003619bd515 to e88d56cdc1500da91142c11191b486fd1666a787

New commits:

e88d56cFixed broken doctest in auto-generationy

comment:44 Changed 3 years ago by jdemeyer

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 jdemeyer

  • Branch changed from u/defeo/ticket/20241 to u/jdemeyer/ticket/20241

comment:46 Changed 3 years ago by jdemeyer

  • Commit changed from e88d56cdc1500da91142c11191b486fd1666a787 to 57e1cbaf2c5a4d43d3796037577696a709a1138d

Merged latest develop.


New commits:

57e1cbaMerge tag '7.4.rc0' into t/20241/ticket/20241

comment:47 Changed 3 years ago by jdemeyer

  • Dependencies #21158 deleted
  • Reviewers set to Jeroen Demeyer

comment:48 Changed 3 years ago by git

  • Commit changed from 57e1cbaf2c5a4d43d3796037577696a709a1138d to 6ef8de2e479449507cf2fddcc67c3b0733820fd9

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

6ef8de2Cosmetic changes to sage.libs.pari.convert

comment:49 follow-up: Changed 3 years ago by 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).

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 git

  • Commit changed from 6ef8de2e479449507cf2fddcc67c3b0733820fd9 to fb54db3ed52b3718cb9aef8eec29101c5ce5dadd

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

fb54db3Cosmetic changes to sage.libs.pari.convert

comment:51 Changed 3 years ago by git

  • Commit changed from fb54db3ed52b3718cb9aef8eec29101c5ce5dadd to 686d2997e355918a4b73608ebd87638391eb4580

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

686d299Split convert_sage in convert_flint and convert_gmp

comment:52 Changed 3 years ago by git

  • Commit changed from 686d2997e355918a4b73608ebd87638391eb4580 to 54117801c99e58bf3ab0e981d3719ea11704fa4c

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

5411780Undo moving of imports in gentoobj()

comment:53 in reply to: ↑ 49 ; follow-up: Changed 3 years ago by defeo

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 git

  • Commit changed from 54117801c99e58bf3ab0e981d3719ea11704fa4c to d522155ff2f79eed03f73a637d8164cff9cf6f6c

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

76db247pari_objtogen -> objtogen
4d2ad6aMinor cleanup in pari imports
d522155Use "self" to name self argument

comment:55 in reply to: ↑ 53 Changed 3 years ago by jdemeyer

Replying to defeo:

If they where in CyPari?, then CyPari? would need GMP/Flint in order to compile, wouldn't it?

Yes, but those could be optional dependencies (like optional packages in Sage).

comment:56 Changed 3 years ago by jdemeyer

Also, cysignals currently has an optional dependency on PARI. It seems to work fine.

comment:57 Changed 3 years ago by jdemeyer

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 self-contained)?

comment:58 Changed 3 years ago by defeo

I think commit d522155ff2f79eed03f73a637d8164cff9cf6f6c is a little brittle.

If I understand auto-generation 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 follow-up: Changed 3 years ago by defeo

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 follow-up: Changed 3 years ago by 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?

comment:61 in reply to: ↑ 59 ; follow-up: Changed 3 years ago by jdemeyer

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 ; follow-up: Changed 3 years ago by defeo

Replying to jdemeyer:

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?

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 defed or cpdefed 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 ; follow-up: Changed 3 years ago by jdemeyer

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 ; follow-up: Changed 3 years ago by defeo

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

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 ; follow-up: Changed 3 years ago by defeo

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 the zero() 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 follow-up: Changed 3 years ago by git

  • Commit changed from d522155ff2f79eed03f73a637d8164cff9cf6f6c to 6f815cd0dcba998a748b59500d3820ff2efc9716

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

6f815cdPut back convert_code for PariInstanceArgument

comment:68 in reply to: ↑ 66 ; follow-up: Changed 3 years ago by 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 than pari_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.

Last edited 3 years ago by jdemeyer (previous) (diff)

comment:69 in reply to: ↑ 60 Changed 3 years ago by jdemeyer

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 git

  • Commit changed from 6f815cd0dcba998a748b59500d3820ff2efc9716 to 8b3013e1c2b698a5cd6bedef03bf90cc29cbb29d

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

7278bcfImprove docstrings
8b3013eChange rational_matrix() nr/nc arguments to "long" to match PARI

comment:71 Changed 3 years ago by jdemeyer

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 ; follow-up: Changed 3 years ago by defeo

Replying to git:

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

6f815cdPut 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 defeo

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 than pari_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.

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 if-branch in a function that is probably orders of magnitude slower than the python-call 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 jdemeyer

Replying to defeo:

what's the rationale behind writing

def ...(self, ...):
    cdef PariInstance pari_instance = <PariInstance>self

instead 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 git

  • Commit changed from 8b3013e1c2b698a5cd6bedef03bf90cc29cbb29d to 62f227ebaf34601aa818b60ff6bf2b3d0641a953

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

62f227ecimport pari_instance as pari

comment:76 Changed 3 years ago by defeo

  • Status changed from needs_review to positive_review

Sorry for the delay, good to go for me.

comment:77 Changed 3 years ago by jdemeyer

Thanks! Could you also have a look at #21703 please?

comment:78 Changed 3 years ago by defeo

yup. looking at it.

comment:79 Changed 3 years ago by vbraun

  • Branch changed from u/jdemeyer/ticket/20241 to 62f227ebaf34601aa818b60ff6bf2b3d0641a953
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.