Skip to content

Set cython directive binding=True #40341

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 19 commits into from
Aug 2, 2025
Merged

Set cython directive binding=True #40341

merged 19 commits into from
Aug 2, 2025

Conversation

tobiasdiez
Copy link
Contributor

@tobiasdiez tobiasdiez commented Jun 30, 2025 •
edited
Loading

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 GitHub Actions
Copy link

github-actions bot commented Jun 30, 2025 •
edited
Loading

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

tobiasdiez added 2 commits July 1, 2025 14:08
@tobiasdiez
Remove deprecated dependent_r_sets
49072ce 
To fix   File "matroid.pyx", line 2698, in init sage.matroids.matroid
NameError: name 'dependent_sets' is not defined
@tobiasdiez
Fix more doctests
741cd1f
@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 •
edited
Loading

Thanks!

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

dcoudert
dcoudert approved these changes Jul 29, 2025
View reviewed changes
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
This was referenced Aug 2, 2025
Closed
Open
@user202729 user202729 mentioned this pull request Aug 16, 2025
Open
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kwankyu kwankyu Awaiting requested review from kwankyu

1 more reviewer

@dcoudert dcoudert dcoudert approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@tobiasdiez @dcoudert @vbraun @kwankyu