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

Upstream Python distribution package for conway_polynomials #32747

Closed
mkoeppe opened this issue Oct 23, 2021 · 22 comments · Fixed by #36765
Closed

Upstream Python distribution package for conway_polynomials #32747

mkoeppe opened this issue Oct 23, 2021 · 22 comments · Fixed by #36765

Comments

@mkoeppe
Copy link
Contributor

mkoeppe commented Oct 23, 2021

... so that we can replace the Sage-specific installation script build/pkgs/conway_polynomials/spkg-install.py by just pip install ...

CC: @orlitzky @antonio-rojas

Component: packages: standard

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

@mkoeppe mkoeppe added this to the sage-9.5 milestone Oct 23, 2021
@mkoeppe mkoeppe modified the milestones: sage-9.5, sage-9.6 Dec 18, 2021
@mkoeppe mkoeppe modified the milestones: sage-9.6, sage-9.7 Mar 5, 2022
@mkoeppe mkoeppe modified the milestones: sage-9.7, sage-9.8 Aug 31, 2022
@mkoeppe mkoeppe modified the milestones: sage-9.8, sage-9.9 Jan 7, 2023
@mkoeppe mkoeppe removed this from the sage-10.0 milestone Apr 30, 2023
@orlitzky
Copy link
Contributor

These polynomials (by way of the same database) are all included with GAP. I'll create a PR that replaces our implementation with GAP's, and deprecates the sage module.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Nov 21, 2023

I don't think that's a good idea from the modularization side of things. GAP is huge, this database is small and is needed for basic ring implementations already

@orlitzky
Copy link
Contributor

Which piece of sage is going to wind up with the GAP dependency?

Punting to GAP does have another benefit. Our existing implementation throws a RuntimeError if you ask it for a polynomial that isn't in the database, but GAP will compute it for you.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Nov 21, 2023

I'm not opposed to using GAP for providing additional functionality (providing Conway polynomials when it can, when our own code would have to fall back to pseudo-Conway).

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Nov 21, 2023

Which piece of sage is going to wind up with the GAP dependency?

Looking for files all__sagemath_gap.py in #36676 (split out from #35095) gives a pretty good overview.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Nov 21, 2023

I see the issue with the missing Conway database in testing distributions like sagemath-categories, sagemath-modules, etc. in #35095 -- large parts of Sage, basically everything that does not have to use non-abelian groups or the Universal Cyclotomic Field.

@orlitzky
Copy link
Contributor

Ok, it was too good to be true I guess. I'll start reverse engineering the tarball on files.sagemath.org.

@orlitzky
Copy link
Contributor

https://github.com/orlitzky/conway-polynomials/

Create a repo under the sage umbrella for it?

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Nov 22, 2023

Yes please!

Should the package be called "sagemath-database-conway-polynomials", as for example Debian calls it according to https://repology.org/project/sagemath-conway-polynomials/versions?

@orlitzky
Copy link
Contributor

Yes please!

Should the package be called "sagemath-database-conway-polynomials", as for example Debian calls it according to https://repology.org/project/sagemath-conway-polynomials/versions?

I was bikeshedding with myself over this. I don't think the "sagemath-" prefix is appropriate any more because the new package is completely independent of SageMath. It ships Frank Lübeck's upstream data file and produces a vanilla dict of coefficient lists. I do think the name conway-polynomials-database would be more appropriate, in case someone wants to create a python package that actually does something with Conway polynomials. But then since this is a continuation of our existing conway_polynomials package with the same version scheme, I decided to stop thinking about it and leave the name alone.

If you think it's better with "-database" at the end, do that and I'll update the package.

@orlitzky
Copy link
Contributor

I forgot perhaps the strongest argument in favor of conway-polynomials: it's obvious that you should import conway_polynomials to use it.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Nov 22, 2023

This all makes sense, thanks for the discussion. conway_polynomials is good.

@orlitzky
Copy link
Contributor

If someone can create either https://github.com/sagemath/conway-polynomials or https://gitlab.com/sagemath/conway-polynomials and give me push access, I'll update the project URLs, date the NEWS entry, tag a release, and start working on the sage PR.

I'm happy to make the pypi release too, however that would work.

(I've preferred a hyphen over an underscore in the project name because otherwise, the underscore has to be normalized to a hyphen anyway to arrive at e.g. the correct pypi URL; https://peps.python.org/pep-0503/#normalized-names)

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Nov 22, 2023

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Nov 22, 2023

I'm happy to make the pypi release too, however that would work.

I can add a GH Actions workflow that pushes to PyPI using the sagemath organization's credentials

@orlitzky
Copy link
Contributor

https://github.com/sagemath/conway-polynomials is yours

Thanks, I just pushed everything.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Nov 23, 2023

I'm happy to make the pypi release too, however that would work.

I can add a GH Actions workflow that pushes to PyPI using the sagemath organization's credentials

Added.

OK to force-push the release tag to trigger the push to PyPI?

@orlitzky
Copy link
Contributor

OK to force-push the release tag to trigger the push to PyPI?

The first release is always missing some imporant file, but maybe this time will be different. AFAIK it's ready.

@orlitzky
Copy link
Contributor

The first release is always missing some imporant file, but maybe this time will be different. AFAIK it's ready.

And now that it's published I see that a tuple would be much more appropriate than a list for the coefficients.

@orlitzky
Copy link
Contributor

I tagged a v0.7 if you could please re-push it.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Nov 24, 2023

vbraun pushed a commit to vbraun/sage that referenced this issue Dec 10, 2023
sagemathgh-36765: Update to new conway-polynomials python package
    
Fix sagemath#32747 by switching to the
newly-minted [conway-polynomials
package](https://pypi.org/project/conway-polynomials/0.7/) on pypi.

IMO `sage.databases.conway` (which makes the dict immutable by wrapping
it in a class) is of dubious value but I've left everything alone for
now.
    
URL: sagemath#36765
Reported by: Michael Orlitzky
Reviewer(s): François Bissey, Matthias Köppe, Michael Orlitzky, Tobias Diez
vbraun pushed a commit to vbraun/sage that referenced this issue Dec 13, 2023
sagemathgh-36765: Update to new conway-polynomials python package
    
Fix sagemath#32747 by switching to the
newly-minted [conway-polynomials
package](https://pypi.org/project/conway-polynomials/0.7/) on pypi.

IMO `sage.databases.conway` (which makes the dict immutable by wrapping
it in a class) is of dubious value but I've left everything alone for
now.
    
URL: sagemath#36765
Reported by: Michael Orlitzky
Reviewer(s): François Bissey, Matthias Köppe, Michael Orlitzky, Tobias Diez
@mkoeppe mkoeppe added this to the sage-10.3 milestone Dec 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants