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

Demote brial (= polybori) from standard to optional, add distribution sagemath-brial, enlarge sagemath-objects, sagemath-categories #36380

Open
wants to merge 60 commits into
base: develop
Choose a base branch
from

Conversation

mkoeppe
Copy link
Member

@mkoeppe mkoeppe commented Oct 1, 2023

as proposed in https://groups.google.com/g/sage-devel/c/ewHP20bmskQ, because (among other things):
It has been dropped from Debian testing (debian-trixie) and ubuntu-noble, where it seems to block SageMath upgrades (Sage is stuck at 9.5 in Debian/Ubuntu). See https://repology.org/project/brial/versions

The new distribution builds on top of enlarged distributions sagemath-objects, sagemath-categories. This is a major update of these distributions, which makes them much more useful.

Also includes style changes to all*.py files cherry-picked from #37900.

📝 Checklist

  • The title is concise, informative, and self-explanatory.
  • The description explains in detail what this PR is about.
  • I have linked a relevant issue or discussion.
  • I have created tests covering the changes.
  • I have updated the documentation accordingly.

⌛ Dependencies

@dimpase
Copy link
Member

dimpase commented Oct 2, 2023

docs don't build. I think one can't build boolean polynomials' docs without brial installed, so this has to be moved out:

--- a/src/doc/en/reference/polynomial_rings/index.rst
+++ b/src/doc/en/reference/polynomial_rings/index.rst
@@ -62,12 +62,4 @@ Infinite Polynomial Rings
    sage/rings/polynomial/symmetric_ideal
    sage/rings/polynomial/symmetric_reduction
 
-Boolean Polynomials
--------------------
-
-.. toctree::
-   :maxdepth: 1
-
-   sage/rings/polynomial/pbori/pbori
-
 .. include:: ../footer.txt

@mkoeppe
Copy link
Member Author

mkoeppe commented Oct 2, 2023

Right. We will need to find a solution for this more general problem of building the manual with optional parts...

@dimpase
Copy link
Member

dimpase commented Oct 2, 2023

also:

src/sage/features/sagemath.py:867: UserWarning: Feature sage.rings.polynomial.pbori is declared standard, but it is provided by sagemath_brial, which is declared experimental in SAGE_ROOT/build/pkgs
  JoinFeature.__init__(self, 'sage.rings.polynomial.pbori',

@dimpase
Copy link
Member

dimpase commented Oct 3, 2023

there are several unconditional imports of BooleanPolynomials left, e.g. in
src/sage/rings/polynomial/multi_polynomial_libsingular.pyx

Should it go via an abstract class, etc, to achieve modularity ?

@mkoeppe
Copy link
Member Author

mkoeppe commented Oct 3, 2023

there are several unconditional imports of BooleanPolynomials left, e.g. in src/sage/rings/polynomial/multi_polynomial_libsingular.pyx

Should it go via an abstract class, etc, to achieve modularity ?

I think I have already done all necessary changes of this kind in #35095. I'll cherry-pick them to this branch later today.

@mkoeppe
Copy link
Member Author

mkoeppe commented Oct 3, 2023

Actually I created the ABC BooleanPolynomialRing_base for the parent, but not for the element class. So if you have chance, a modularizing fix here would be welcome

Edit: Done already in f5c6196

.gitignore Outdated
@@ -215,6 +217,7 @@ build/bin/sage-build-env-config
/pkgs/sagemath-categories/requirements.txt
/pkgs/sagemath-environment/requirements.txt
/pkgs/sagemath-repl/requirements.txt
/pkgs/sagemath-brial/requirements*.txt
/pkgs/sagemath-categories/MANIFEST.in

Copy link
Collaborator

Choose a reason for hiding this comment

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

OK for now. But I think this block of lines should be sorted out by distribution names...

.gitignore Outdated Show resolved Hide resolved
@kwankyu
Copy link
Collaborator

kwankyu commented Oct 18, 2023

I get

$ ./sage -sh -c '(cd pkgs/sagemath-brial && tox -v -v -v -e sagepython)'
...
      
            cypari2/closure.pyx:139:4: 'sig_on' is not a constant, variable or function identifier
      
            Error compiling Cython file:
            ------------------------------------------------------------
            ...
      
            cdef int _pari_init_closure() except -1:
                sig_on()
                global ep_call_python
                ep_call_python = install(<void*>call_python, "call_python", 'DGDGDGDGDGD5,U,U')
                sig_off()
                ^
            ------------------------------------------------------------
      
            cypari2/closure.pyx:142:4: 'sig_off' is not a constant, variable or function identifier
      
            Error compiling Cython file:
            ------------------------------------------------------------
            ...
                # Only 5 arguments are supported for now...
                if nargs > 5:
                    nargs = 5
      
                # Fill in default arguments of PARI function
                sig_on()
                ^
            ------------------------------------------------------------
      
            cypari2/closure.pyx:230:4: 'sig_on' is not a constant, variable or function identifier
            Traceback (most recent call last):
...
              File "/private/var/folders/td/fw1q9ljs311ggyph77rs53_40000gn/T/pip-install-h6yasewl/cypari2_0d737fa76b3f479ea5e1b6ab73dffe77/.eggs/Cython-3.0.4-py3.11.egg/Cython/Build/Dependencies.py", line 1321, in cythonize_one
                raise CompileError(None, pyx_file)
            Cython.Compiler.Errors.CompileError: cypari2/closure.pyx
            [end of output]
      
        note: This error originates from a subprocess, and is likely not a problem with pip.
      error: legacy-install-failure
      
      × Encountered error while trying to install package.
      ╰─> cypari2
      
      note: This is an issue with the package mentioned above, not pip.
      hint: See above for output from the failure.
      [end of output]
  
  note: This error originates from a subprocess, and is likely not a problem with pip.
error: subprocess-exited-with-error

× pip subprocess to install build dependencies did not run successfully.
│ exit code: 1
╰─> See above for output.

note: This error originates from a subprocess, and is likely not a problem with pip.
sagepython: 67309 C exit 1 (66.94 seconds) /Users/kwankyu/GitHub/sage-dev/pkgs/sagemath-brial> python -I -m pip install -r requirements.txt pid=54141 [tox/execute/api.py:275]
  sagepython: FAIL code 1 (66.95 seconds)
  evaluation failed :( (67.15 seconds)

@kwankyu
Copy link
Collaborator

kwankyu commented Oct 18, 2023

This src/sage_setup/__init__.py seems new (or copied from somewhere?) What is the intention? Should this

sage: sage_setup.sage_setup(['sagemath-modules'])

work? There's no doc for it...

@github-actions
Copy link

github-actions bot commented Oct 23, 2023

Documentation preview for this PR (built with commit a019633; changes) is ready! 🎉
This preview will update shortly after each push to this PR.

vbraun pushed a commit to vbraun/sage that referenced this pull request Oct 24, 2023
    
<!-- ^^^^^
Please provide a concise, informative and self-explanatory title.
Don't put issue numbers in there, do this in the PR body below.
For example, instead of "Fixes sagemath#1234" use "Introduce new method to
calculate 1+1"
-->
<!-- Describe your changes here in detail -->

<!-- Why is this change required? What problem does it solve? -->
Documentation is conditionalized by using Sphinx tags.

Split out from (and preparation for):
- sagemath#36380

Part of:
- sagemath#29705

<!-- If this PR resolves an open issue, please link to it here. For
example "Fixes sagemath#12345". -->
<!-- If your change requires a documentation PR, please link it
appropriately. -->

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->
<!-- If your change requires a documentation PR, please link it
appropriately -->
<!-- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->
<!-- Feel free to remove irrelevant items. -->

- [x] The title is concise, informative, and self-explanatory.
- [ ] The description explains in detail what this PR is about.
- [x] I have linked a relevant issue or discussion.
- [ ] I have created tests covering the changes.
- [x] I have updated the documentation accordingly.

### ⌛ Dependencies

<!-- List all open PRs that this PR logically depends on
- sagemath#12345: short description why this is a dependency
- sagemath#34567: ...
-->

<!-- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->
    
URL: sagemath#36495
Reported by: Matthias Köppe
Reviewer(s): Kwankyu Lee
@mkoeppe
Copy link
Member Author

mkoeppe commented Jun 11, 2024

If it helps, I can rebase so that all these are added in the first or in the last commit, or something like that, let me know

Rebased. I haven't added the sage_setup: distributions for now, for ease of reviewing.

@mkoeppe
Copy link
Member Author

mkoeppe commented Jun 11, 2024

git diff 7ef824de14a3b31462ba97fb1d2076642ed6cf4a...a0196331926 will show the changes on top of the two merged dependencies.

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

Successfully merging this pull request may close these issues.

None yet

5 participants