Skip to content

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

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

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

Conversation

@mkoeppe

@mkoeppe mkoeppe commented Jun 7, 2023

Copy link
Copy Markdown
Contributor

📚 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

mkoeppe commented Jun 7, 2023

Copy link
Copy Markdown
Contributor Author

@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

dcoudert commented Jun 8, 2023

Copy link
Copy Markdown
Collaborator

@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

mkoeppe commented Jun 8, 2023

Copy link
Copy Markdown
Contributor Author

OK, done in fe07439.

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

@github-actions

github-actions Bot commented Jun 8, 2023

Copy link
Copy Markdown

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

@dcoudert dcoudert left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM.

@mkoeppe

mkoeppe commented Jun 9, 2023

Copy link
Copy Markdown
Contributor Author

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants