Autogeneration of pari declarations

[videlec]

Issue #27 shows that it will be impossible to support various pari versions without auto generating pari declarations. The branch is a proposal to have an autogenerated cypari2/auto_paridecl.pxi, generated from the PARI .h files, which is automatically included in cypari2/paridecl.pxd via a Cython include directive.

The branch makes cypari2 works with both pari 2.9.3 and pari master at a8e4b6c46.

[videlec]

This is too weak.

[videlec]

This is not robust! A declaration like long *lambdaf(void); would be wrongly modified! Would be better to check for the specific line it is intended for

GEN     ZX_ZXY_rnfequation(GEN A, GEN B, long *lambda);

[jpflori]

Maybe the "cdef extern from *" could be made more explicit?
And the lambda renaming stuff more general "just in case"?

[videlec]

Maybe the "cdef extern from *" could be made more explicit?

It could. However, having this more explicit will include a #include <paridecl.h> in the C file which is already done by types.pxd. It is as it used to be.

And the lambda renaming stuff more general "just in case"?

@jpflori I would be happy with something more general. However, you need to catch or not catch various subtle differences. Namely catch

void f(int lambda)
void f(int *lambda)
void f(int lambda, int a)
void f(int *lambda, int a)

but do not catch

void f(int lambda1)
void my_lambda(void)

Do you think it is worth the trouble?

[jdemeyer]

The branch is a proposal to have an autogenerated cypari2/auto_paridecl.pxi that will be included in cypari2/paridecl.pxd.

I would keep these two independent. Keep the old paridecl.pxd and use the new auto_paridecl.pxd only for the auto-generated code.

[videlec]

The branch is a proposal to have an autogenerated cypari2/auto_paridecl.pxi
that will be included in cypari2/paridecl.pxd.

I would keep these two independent. Keep the old paridecl.pxd and use the new
auto_paridecl.pxd only for the auto-generated code.

What do you mean!? You do not want to use two include directives for the pari declarations. So one has to include the other in a way or the other (can be include auto_paridecl.pxi or from auto_paridecl.pxi cimport *).

Perhaps the description is not clear enough.

[jdemeyer]

I honestly don't think that "parsing" the .h file without a real C parser is a good idea. You're making a lot of assumptions which are satisfied for now, but which could easily change in the future.

Sorry, but -1 to this idea.

[videlec]

I honestly don't think that "parsing" the .h file without a real C parser is a good idea. You're making a lot of assumptions which are satisfied for now, but which could easily change in the future. Sorry, but -1 to this idea.

I can change the code to use a proper C parser.

Not talking about the method: how would you solve having cypari2 working with different versions of Pari without this auto-generation?

[jdemeyer]

Not talking about the method: how would you solve having cypari2 working with different versions of Pari without this auto-generation?

As explained in my recent e-mail, I would still auto-generate the declarations, but from the pari.desc file instead of the header files.

The advantage is that pari.desc has a well-defined and simple structure and we already have code to parse it.

The disadvantage is that it only contains declarations for functions which have a GP interface, so we would still need to keep the old paridecl.pxd. That's what I meant in #28 (comment): keep paridecl.pxd exactly as it is right now and only use the new auto_decl.pxd inside gen.pyx and pari_instance.pyx.

[videlec]

Not talking about the method: how would you solve having cypari2 working with different versions of Pari without this auto-generation?

As explained in my recent e-mail, I would still auto-generate the declarations, but from the pari.desc file instead of the header files. The advantage is that pari.desc has a well-defined and simple structure and we already have code to parse it. The disadvantage is that it only contains declarations for functions which have a GP interface, so we would still need to keep the old paridecl.pxd. That's what I meant in #28 (comment): keep paridecl.pxd exactly as it is right now and only use the new auto_decl.pxd inside gen.pyx and pari_instance.pyx.

What is the point of keeping paridecl.pxd as it is? This file would still contain erroneous declarations when using the "bad" version of pari (e.g. pari master right now).

[jdemeyer]

What is the point of keeping paridecl.pxd as it is?

The main point would be to support packages which directly call the PARI library functions, without going through the Pari or Gen classes. For example, that is what is done in SageMath for finite field elements.

This file would still contain erroneous declarations when using the "bad" version of pari (e.g. pari master right now).

I think it would make sense to keep the declarations of the latest stable version there. Sure, there will be incompatibilities with PARI master. However, even if we put the correct auto-generated declarations there, there will still be incompatibilities with the code using those declarations.

In other words, there are 3 places where the functions appear:

1. PARI header files
2. Cython declarations in cypari2
3. User code (for example SageMath)

We can only control 2, so there is nothing we can do with incompatibilities between 1 and 3. If 1 and 3 are incompatible, I think it's pointless to worry whether 2 should be compatible with 1 and incompatible with 3 or the other way around.

[videlec]

What is the point of keeping paridecl.pxd as it is?

The main point would be to support packages which directly call the PARI library functions, without going through the Pari or Gen classes. For example, that is what is done in SageMath for finite field elements.

But then the function signatures must be correct!

This file would still contain erroneous declarations when using the "bad" version of pari (e.g. pari master right now).

I think it would make sense to keep the declarations of the latest stable version there. Sure, there will be incompatibilities with PARI master. However, even if we put the correct auto-generated declarations there, there will still be incompatibilities with the code using those declarations.

In other words, there are 3 places where the functions appear:

1. PARI header files
2. Cython declarations in cypari2
3. User code (for example SageMath)

We can only control 2, so there is nothing we can do with incompatibilities between 1 and 3. If 1 and 3 are incompatible, I think it's pointless to worry whether 2 should be compatible with 1 and incompatible with 3 or the other way around.

I agree that it is pointless to worry about incompatibilities between 1 and 3 because whatever we do that would never work out. But we should definitely support user basis programs that have Cython code up to date with their Pari versions. It is even possible for user code to have programs compatible with both PARI master and PARI stable using macros. In other words, I strongly think that our declarations (point 2) should match whatever PARI version is to be encountered on the system (point 1)... which is what I was trying to achieve.

[jdemeyer]

I agree that it is pointless to worry about incompatibilities between 1 and 3 because whatever we do that would never work out. But we should definitely support user basis programs that have Cython code up to date with their Pari versions.

Well, you should also definitely support user basis programs that have Cython code up to date with the latest stable PARI.

It is even possible for user code to have programs compatible with both PARI master and PARI stable using macros.

If user code does that, they are surely clever enough to not need the declarations from cypari2. It's not particularly hard for user code to write

cdef extern from *:
   GEN some_pari_function(GEN)

if the declarations from cypari2 are not sufficient. So I think it's fine to support just latest stable. Let me also recall that the difference is not that big: there is only a handful of functions which changed. So we are almost certainly arguing about the empty set (programs which really need the declarations from PARI master).

[videlec]

I agree that it is pointless to worry about incompatibilities between 1 and 3 because whatever we do that would never work out. But we should definitely support user basis programs that have Cython code up to date with their Pari versions.

Well, you should also definitely support user basis programs that have Cython code up to date with the latest stable PARI.

Isn't it exactly what I said? "Their PARI version" meant "whatever PARI the users run".

It's not particularly hard for user code to write

cdef extern from *:
    GEN some_pari_function(GEN)

if the declarations from cypari2 are not sufficient.

If it is not particularly hard to write cdef extern from * what is the point of shipping paridecl.pxd at all then? The only reason I see is to avoid code duplication (and possible copy/paste mistakes). In other words to simplfy the life of users. Which is in the favor of having the declarations up to date with whatever PARI version is on the system.

Let me also recall that the difference is not that big: there is only a handful of functions which changed. So we are almost certainly arguing about the empty set (programs which really need the declarations from PARI master).

If you think it is not worth it you can stop arguing ;-)

For the sake of this pull request I propose to leave the question of paridecl.pxd for now and concentrate on auto_paridecl.pxd which will be the only Cython header used in both gen.pyx and pari_instance.pyx.

[videlec]

I don't even see how to make your proposal works. There are a lot (> 100) of pari function calls in gen.pyx that do not get auto generated from the prototypes in pari.desc. What do you propose for them? Should we cimport only the relevant functions?

[videlec]

@jdemeyer All right. I came up with someting... I am compiling pari master to see whether it works also there.

[videlec]

Build passes on pari master (and only got 12 doctest failures)

[jpflori]

I would say I'm rather on Vincent side here.
I'm not shocked at all by using the paridecl.h and trying to automagicaly convert it to a pxd file, whether its with sed or using a C preprocessor, though at least in paridecl.h case the file seems to be well maintained (just like the .desc file) and we need not all the power of the C preprocessor.
This semi manual conversions of C header to pxd files is a pain, so if in some cases we can automate it, it is more than welcome...

As far as my first remarks are concerned, I would still cdef extern from paridecl.h just in case the other pxd file disappear, and extend the lambda hack to be more general maybe putting it in a separate "fix" method which could be extended with other needed hacks.

[jdemeyer]

at least in paridecl.h case the file seems to be well maintained (just like the .desc file) and we need not all the power of the C preprocessor.

I feel like this is true by accident and not something that we can/should rely on. Now if you could somehow get a guarantee from the PARI developers that they will stick with a very simply syntax for paridecl.h which could get parsed by us, you might convince me.

[jdemeyer]

what is the point of shipping paridecl.pxd at all then?

For the easy cases. If some package intends to be compatible with PARI stable, they can just use our declarations from cypari2. Now, whether such a package is compatible with PARI master is beyond our control. As I explained before, auto-generating a correct paridecl.pxd in this case won't help to fix such incompatibilities (which are between the user code and the PARI library).

The hard case is where a package needs a function from PARI master which is different or non-existent in PARI stable. In this case, the package needs to special-case depending on the PARI version anyway (either it needs different code for PARI stable or it won't work at all on PARI stable). I think it's fine if cypari2 does not support that out of the box, since it is sufficiently easy for the user code to support that.

[videlec]

@jdemeyer And what about the current pull request which at least allows to build cypari2 with pari master? (at 1f3fca2)

[jdemeyer]

@jdemeyer And what about the current pull request which at least allows to build cypari2 with pari master? (at 1f3fca2)

Well, that is the approach that I meant, so I agree. However, it seems that you were not totally convinced and @jpflori wasn't either, so I felt that the discussion was not finished. If you are in Bordeaux, maybe this is something you could discuss with the PARI developers too.

[jdemeyer]

About the patch itself: I don't understand why you are blacklisting all the plotting functions. Those functions might actually be useful, especially in interactive environments where plotting makes sense.

[videlec]

About the patch itself: I don't understand why you are blacklisting all the plotting functions. Those functions might actually be useful, especially in interactive environments where plotting makes sense.

Because I got errors if I allow them. They were not supported before anyway. If you want to work on it, please open another pull request.

[videlec]

@jdemeyer And what about the current pull request which at least allows to build cypari2 with pari master? (at 1f3fca2)

Well, that is the approach that I meant, so I agree. However, it seems that you were not totally convinced and @jpflori wasn't either, so I felt that the discussion was not finished. If you are in Bordeaux, maybe this is something you could discuss with the PARI developers too.

I am indeed not convinced but I like much more the branch than the cypari2 master. In Bordeaux it is holidays until mid-September. I will definitely discuss it with Bill and Karim. In the mean time, it seems reasonable to me that we merge this PR.

[jdemeyer]

Because I got errors if I allow them. They were not supported before anyway. If you want to work on it, please open another pull request.

It's true that plotting has changed significantly since the latest stable PARI release. If you are getting errors, let's look at that later.

[videlec]

@jdemeyer @defeo do you agree to merge this pull request?

@jdemeyer A release is explicitely asked in Sage at https://trac.sagemath.org/ticket/23518. Could you make a new one (hopefully including this PR)?

[jdemeyer]

I just pushed a commit with a few fixes. Vincent: If you accept these, then this can be merged for me.