sagemath / sage

Main repository of SageMath
https://www.sagemath.org
Other
1.38k stars 470 forks source link

Deprecate sage.interfaces.primecount #32412

Closed mkoeppe closed 3 years ago

mkoeppe commented 3 years ago

(see #25009 comment:4)

CC: @videlec @tscrim

Component: packages: optional

Author: Matthias Koeppe

Branch: 20e8d20

Reviewer: Travis Scrimshaw

Issue created by migration from https://trac.sagemath.org/ticket/32412

mkoeppe commented 3 years ago

Branch: u/mkoeppe/deprecate_sage_interfaces_primecount

mkoeppe commented 3 years ago

Author: Matthias Koeppe

mkoeppe commented 3 years ago

Commit: 85c63e9

mkoeppe commented 3 years ago

New commits:

85c63e9sage.interfaces.primecount: Deprecate
tscrim commented 3 years ago

Reviewer: Travis Scrimshaw

tscrim commented 3 years ago
comment:3

Just to confirm, we are deprecating the interface in favor of only having the libs/primecount.pxd, correct? If so, LGTM and you can set a positive review.

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 3 years ago

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

20e8d20src/sage/libs/primecount.pxd: Add comment indicating deprecation
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 3 years ago

Changed commit from 85c63e9 to 20e8d20

mkoeppe commented 3 years ago
comment:5

Actually, the pxd file would also be scheduled for removal.

tscrim commented 3 years ago
comment:6

Okay, then I am a bit confused about this now. We still want to link to the library, right? If we don't have a lib or interface, then how will we access it within Sage?

mkoeppe commented 3 years ago
comment:7

Note that it is currently unused in Sage.

sage.interfaces.primecount, because its compilation depends on an optional package, would have to be provided by a distribution separate from sagemath-standard.

For other modules such as sage.graphs.graph_decompositions.tdlib that depend on an optional package, we would indeed split it out as a distribution sagemath-tdlib. This is done in #29864. Note that sage.graphs.graph_decompositions.tdlib depends on both Sage (by using the Graph class) and on the optional package.

The difference is that sage.interfaces.primecount does not depend in any way on Sage. Therefore it should be redone as a standalone Python (Cython) package, in the same way that Vincent split out (and extended) https://pypi.org/project/pplpy/ from Sage (sage.libs.ppl).

tscrim commented 3 years ago
comment:8

Replying to @mkoeppe:

For other modules such as sage.graphs.graph_decompositions.tdlib that depend on an optional package, we would indeed split it out as a distribution sagemath-tdlib. This is done in #29864. Note that sage.graphs.graph_decompositions.tdlib depends on both Sage (by using the Graph class) and on the optional package.

Thank you for the explanation. Does that mean this distribution meant to replace the current package? Otherwise it feels like double duty as there already is the package itself on top of the library. (My standard example of this is meataxe.)

The difference is that sage.interfaces.primecount does not depend in any way on Sage. Therefore it should be redone as a standalone Python (Cython) package, in the same way that Vincent split out (and extended) https://pypi.org/project/pplpy/ from Sage (sage.libs.ppl).

So the expectation here is that such a standalone package would provide a way for it to be important from Python with a usual import statement?

mkoeppe commented 3 years ago
comment:9

Replying to @tscrim:

Replying to @mkoeppe:

For other modules such as sage.graphs.graph_decompositions.tdlib that depend on an optional package, we would indeed split it out as a distribution sagemath-tdlib. This is done in #29864. Note that sage.graphs.graph_decompositions.tdlib depends on both Sage (by using the Graph class) and on the optional package.

Thank you for the explanation. Does that mean this distribution meant to replace the current package? Otherwise it feels like double duty as there already is the package itself on top of the library. (My standard example of this is meataxe.)

There will always be two packages - one providing the C library and one providing the Python bindings.

When the Python bindings are not Sage-specific:

When the Python package is Sage-specific:

mkoeppe commented 3 years ago
comment:10

Replying to @tscrim:

The difference is that sage.interfaces.primecount does not depend in any way on Sage. Therefore it should be redone as a standalone Python (Cython) package, in the same way that Vincent split out (and extended) https://pypi.org/project/pplpy/ from Sage (sage.libs.ppl).

So the expectation here is that such a standalone package would provide a way for it to be imported from Python with a usual import statement?

Yes, exactly.

tscrim commented 3 years ago
comment:11

Okay, thank you. LGTM.

mkoeppe commented 3 years ago
comment:12

Thanks!

vbraun commented 3 years ago

Changed branch from u/mkoeppe/deprecate_sage_interfaces_primecount to 20e8d20

dimpase commented 2 years ago

Changed commit from 20e8d20 to none

dimpase commented 2 years ago
comment:14

Let us revert this, upgrade primecount, and use it to compute prime_pi, instead of buggy and slow Sage code - cf. #24960

mkoeppe commented 2 years ago
comment:15

comment:7, 3rd paragraph, explains the correct way forward

dimpase commented 2 years ago
comment:16

Replying to @mkoeppe:

comment:7, 3rd paragraph, explains the correct way forward

I forgot to mention that we should promote primecount to standard. I don't understand why one need to do a special package thing for the (pretty trivial) interface, why can't this just live in sagelib, like other interfaces to C/C++ libs?

mkoeppe commented 2 years ago
comment:17

Replying to @dimpase:

I don't understand why one need to do a special package thing for the (pretty trivial) interface, why can't this just live in sagelib, like other interfaces to C/C++ libs?

Did you read my discussion with tscrim above, on exactly this topic?

dimpase commented 2 years ago
comment:18

Perhaps we can use https://github.com/hearot/primecount-python (not on pip, unfortunately)

dimpase commented 2 years ago
comment:19

Replying to @mkoeppe:

Replying to @dimpase:

I don't understand why one need to do a special package thing for the (pretty trivial) interface, why can't this just live in sagelib, like other interfaces to C/C++ libs?

Did you read my discussion with tscrim above, on exactly this topic?

It was about dependence on an optional package, but we're promoting it to standard now?

mkoeppe commented 2 years ago
comment:20

It doesn't matter that you are promoting it to standard.

dimpase commented 2 years ago
comment:21

anyhow, there is an external package already, it's unfortunately a bit light on the distribution side - well, the author is still in high school according to GH. Maybe he can use some help in building wheels and stuff.