Skip to content

ABC for BooleanPolynomialRing #35240

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Apr 1, 2023
Merged

ABC for BooleanPolynomialRing #35240

merged 2 commits into from
Apr 1, 2023

Conversation

@mkoeppe
Copy link
Contributor

@mkoeppe mkoeppe commented Mar 6, 2023

📚 Description

The new ABC sage.rings.polynomial.multi_polynomial_ring_base.BooleanPolynomialRing_base can be imported for isinstance tests, without a compile time dependency on brial.

This is a step towards a modularized distribution sagemath-brial.

Part of:

📝 Checklist

  • I have made sure that the title is self-explanatory and the description concisely explains the PR.
  • I have linked an issue or discussion.
  • I have created tests covering the changes.
  • I have updated the documentation accordingly.

⌛ Dependencies

kiwifb
kiwifb reviewed Mar 6, 2023
View reviewed changes
src/sage/rings/polynomial/pbori/pbori.pxd

from sage.rings.polynomial.multi_polynomial_ring_base cimport \
MPolynomialRing_base
from sage.rings.polynomial.multi_polynomial_ring_base cimport MPolynomialRing_base, BooleanPolynomialRing_base
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it still necessary to cimport MPolynomialRing_base? It is the only instance in this file. Do any file importing from sage.rings.polynomial.pbori.pbori expect it? If so, could they import it directly from sage.rings.polynomial.multi_polynomial_ring_base instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is used in pbori.pyx for an isinstance check

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can of course move the cimport to the pyx file if you think that's better

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not think it is particularly better. A cython expert would have to chime in on that. So, let's leave it as is.

@kiwifb
Copy link
Member

kiwifb commented Mar 6, 2023

I'll let the automated stuff do its bits before approving.

@github-actions
Copy link

github-actions bot commented Mar 6, 2023

Documentation preview for this PR is ready! 🎉
Built with commit: 77c25aa

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Mar 7, 2023

Tests pass

kiwifb
kiwifb approved these changes Mar 7, 2023
View reviewed changes
Copy link
Member

@kiwifb kiwifb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I cannot see a reason not to approve.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Mar 7, 2023

Thank you!

@vbraun vbraun merged commit 88de5ab into sagemath:develop Apr 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@kiwifb kiwifb kiwifb approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@mkoeppe mkoeppe

Labels

c: commutative algebra t: refactoring

Projects

None yet

Milestone

sage-10.0

Development

Successfully merging this pull request may close these issues.

3 participants

@mkoeppe @kiwifb @vbraun