Skip to content

Conversation

@tobiasdiez
Copy link
Contributor

@tobiasdiez tobiasdiez commented Jun 30, 2025

Continuation of #26254. This is needed for better integration of Cython functions and thereby unlocks a few further documentation improvements (#27578, #30884, #31309, ...).

Moreover, binding=True is the default in Cython 3.

📝 Checklist

  • The title is concise and informative.
  • 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 and checked the documentation preview.

⌛ Dependencies

@github-actions
Copy link

github-actions bot commented Jun 30, 2025

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

To fix   File "matroid.pyx", line 2698, in init sage.matroids.matroid
NameError: name 'dependent_sets' is not defined
@tobiasdiez
Copy link
Contributor Author

@dcoudert The test for del_vertex that you have added in #39271 (and only that test) fails now with binding=True. Do you have an idea why/how to fix this?

sage -t --warn-long 5.0 --random-seed=162462718596100674006260289682106403625 src/sage/graphs/base/static_sparse_backend.pyx
**********************************************************************
Error: Failed example:: Got: 
Traceback (most recent call last):
  File "/Users/runner/miniconda3/envs/sage-dev/lib/python3.11/site-packages/sage/doctest/forker.py", line 730, in _run
    self.compile_and_execute(example, compiler, test.globs)
  File "/Users/runner/miniconda3/envs/sage-dev/lib/python3.11/site-packages/sage/doctest/forker.py", line 1154, in compile_and_execute
    exec(compiled, globs)
  File "<doctest sage.graphs.base.static_sparse_backend.StaticSparseBackend.del_vertex[3]>", line 1, in <module>
    g.del_vertex('a')
  File "static_sparse_backend.pyx", line 1546, in sage.graphs.base.static_sparse_backend.StaticSparseBackend.del_vertex
TypeError: an integer is required

    g.del_vertex('a')
Expected:
    Traceback (most recent call last):
    ...
    ValueError: graph is immutable; please change a copy instead (use function copy())
Got:
    <BLANKLINE>
    Traceback (most recent call last):
      File "/Users/runner/miniconda3/envs/sage-dev/lib/python3.11/site-packages/sage/doctest/forker.py", line 730, in _run
        self.compile_and_execute(example, compiler, test.globs)
      File "/Users/runner/miniconda3/envs/sage-dev/lib/python3.11/site-packages/sage/doctest/forker.py", line 1154, in compile_and_execute
        exec(compiled, globs)
      File "<doctest sage.graphs.base.static_sparse_backend.StaticSparseBackend.del_vertex[3]>", line 1, in <module>
        g.del_vertex('a')
      File "static_sparse_backend.pyx", line 1546, in sage.graphs.base.static_sparse_backend.StaticSparseBackend.del_vertex
    TypeError: an integer is required
**********************************************************************
1 item had failures:
   1 of   9 in sage.graphs.base.static_sparse_backend.StaticSparseBackend.del_vertex
    [198 tests, 1 failure, 0.24s wall]

@dcoudert
Copy link
Contributor

dcoudert commented Jul 1, 2025

Got it, and it's my fault: methods add_vertex and del_vertex are defined twice in StaticSparseBackend.
The solution is to remove the duplicates lines 1512-1546.

@tobiasdiez
Copy link
Contributor Author

Got it, and it's my fault: methods add_vertex and del_vertex are defined twice in StaticSparseBackend. The solution is to remove the duplicates lines 1512-1546.

Thanks a lot, that worked!

@tobiasdiez
Copy link
Contributor Author

The CI runs for sage-the-distro are red, but I think it's just because it doesn't recognize the changed compilation options correctly and does an incremental build, without recompiling the necessary cython modules.

@tobiasdiez tobiasdiez requested review from dcoudert and kwankyu July 28, 2025 11:42
@dcoudert
Copy link
Contributor

This looks good to me but I'm not sure how to double check that on my laptop. Any advise is more than welcome.

@tobiasdiez
Copy link
Contributor Author

tobiasdiez commented Jul 29, 2025

Thanks!

Since all cython modules need to be recompiled, either make sagelib-clean (+ build) or sage -ba should do the trick.

Copy link
Contributor

@dcoudert dcoudert left a comment

Choose a reason for hiding this comment

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

It compiles well on Fedora 39 and passes tests (make ptest).

LGTM.

@dcoudert
Copy link
Contributor

I hope the tests I did are sufficient

@tobiasdiez
Copy link
Contributor Author

Thanks for the review!

@vbraun vbraun merged commit c18bea3 into sagemath:develop Aug 2, 2025
18 of 28 checks passed
vbraun pushed a commit to vbraun/sage that referenced this pull request Sep 11, 2025
sagemathgh-40687: Drop all cython binding=True declarations
    
As this is the default mode since
sagemath#40341
    
URL: sagemath#40687
Reported by: Antonio Rojas
Reviewer(s):
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants