Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Deprecate sage.interfaces.primecount #32412

Closed
mkoeppe opened this issue Aug 24, 2021 · 26 comments
Closed

Deprecate sage.interfaces.primecount #32412

mkoeppe opened this issue Aug 24, 2021 · 26 comments

Comments

@mkoeppe
Copy link
Contributor

mkoeppe commented Aug 24, 2021

(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 mkoeppe added this to the sage-9.5 milestone Aug 24, 2021
@mkoeppe
Copy link
Contributor Author

mkoeppe commented Aug 26, 2021

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Aug 26, 2021

Author: Matthias Koeppe

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Aug 26, 2021

Commit: 85c63e9

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Aug 26, 2021

New commits:

85c63e9sage.interfaces.primecount: Deprecate

@tscrim
Copy link
Collaborator

tscrim commented Aug 26, 2021

Reviewer: Travis Scrimshaw

@tscrim
Copy link
Collaborator

tscrim commented Aug 26, 2021

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.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 27, 2021

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

20e8d20src/sage/libs/primecount.pxd: Add comment indicating deprecation

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 27, 2021

Changed commit from 85c63e9 to 20e8d20

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Aug 27, 2021

comment:5

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

@tscrim
Copy link
Collaborator

tscrim commented Aug 27, 2021

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
Copy link
Contributor Author

mkoeppe commented Aug 27, 2021

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
Copy link
Collaborator

tscrim commented Aug 27, 2021

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
Copy link
Contributor Author

mkoeppe commented Aug 27, 2021

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:

  • ppl / pplpy
  • igraph / python_igraph
  • primecount / pyprimecount (to be created)

When the Python package is Sage-specific:

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Aug 27, 2021

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
Copy link
Collaborator

tscrim commented Aug 27, 2021

comment:11

Okay, thank you. LGTM.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Aug 27, 2021

comment:12

Thanks!

@vbraun
Copy link
Member

vbraun commented Sep 13, 2021

Changed branch from u/mkoeppe/deprecate_sage_interfaces_primecount to 20e8d20

@dimpase
Copy link
Member

dimpase commented Nov 12, 2021

Changed commit from 20e8d20 to none

@dimpase
Copy link
Member

dimpase commented Nov 12, 2021

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
Copy link
Contributor Author

mkoeppe commented Nov 12, 2021

comment:15

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

@dimpase
Copy link
Member

dimpase commented Nov 12, 2021

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
Copy link
Contributor Author

mkoeppe commented Nov 12, 2021

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
Copy link
Member

dimpase commented Nov 12, 2021

comment:18

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

@dimpase
Copy link
Member

dimpase commented Nov 12, 2021

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
Copy link
Contributor Author

mkoeppe commented Nov 12, 2021

comment:20

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

@dimpase
Copy link
Member

dimpase commented Nov 12, 2021

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants