Skip to content

sage.combinat: Split some Cython modules (modularization fixes)#35741

Merged
vbraun merged 8 commits intosagemath:developfrom
mkoeppe:sage_combinat_split_cython_modules
Jun 21, 2023
Merged

sage.combinat: Split some Cython modules (modularization fixes)#35741
vbraun merged 8 commits intosagemath:developfrom
mkoeppe:sage_combinat_split_cython_modules

Conversation

@mkoeppe
Copy link
Copy Markdown
Contributor

@mkoeppe mkoeppe commented Jun 7, 2023 •
edited
Loading

📚 Description

Some basic modules of sage.combinat (.permutation, .combination, etc.) are needed in the distribution sagemath-categories (as of #35095). We make them importable (by splitting a Cython module into several parts and by using lazy imports) and separately testable (using # optional doctest directives).

Likewise, split the parts of sage.combinat.posets.hasse_cython that need the FLINT library out as a separate module (this is for the package sagemath-graphs in #35095)

📝 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

@mkoeppe
Copy link
Copy Markdown
Contributor Author

mkoeppe commented Jun 7, 2023

@dcoudert, from #35564 (comment):

Shouldn't you add # cython: binding=True ?

I don't know; the source file from which I split it out did not have this directive if I'm not mistaken.

I'll be happy to add it to if you recommend it?

@dcoudert
Copy link
Copy Markdown
Collaborator

dcoudert commented Jun 8, 2023

@dcoudert, from #35564 (comment):

Shouldn't you add # cython: binding=True ?

I don't know; the source file from which I split it out did not have this directive if I'm not mistaken.

I'll be happy to add it to if you recommend it?

I had to add # cython: binding=True in some .pyx files in order to import some methods from the console, e.g., with from sage.graphs.graph_decompositions.cutwidth import cutwidth. This is convenient in some cases.

@mkoeppe
Copy link
Copy Markdown
Contributor Author

mkoeppe commented Jun 8, 2023

OK, done in fe07439.

This will be the default anyway in the upcoming Cython 3 (#29863).

@github-actions
Copy link
Copy Markdown

github-actions bot commented Jun 8, 2023

Documentation preview for this PR (built with commit fe07439) is ready! 🎉

dcoudert
dcoudert approved these changes Jun 9, 2023
View reviewed changes
Copy link
Copy Markdown
Collaborator

@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.

LGTM.

@mkoeppe
Copy link
Copy Markdown
Contributor Author

mkoeppe commented Jun 9, 2023

Thanks a lot for reviewing!

@vbraun vbraun merged commit 6252db6 into sagemath:develop Jun 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dcoudert dcoudert dcoudert approved these changes

Assignees

@mkoeppe mkoeppe

Labels

c: combinatorics t: refactoring

Projects

None yet

Milestone

sage-10.1

Development

Successfully merging this pull request may close these issues.

3 participants

@mkoeppe @dcoudert @vbraun