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

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

Merged
merged 8 commits into from
Jun 21, 2023

Conversation

mkoeppe
Copy link
Member

@mkoeppe mkoeppe commented Jun 7, 2023

πŸ“š 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
Member 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
Contributor

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
Member 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

github-actions bot commented Jun 8, 2023

Documentation preview for this PR (built with commit fe07439) is ready! πŸŽ‰

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.

LGTM.

@mkoeppe
Copy link
Member Author

mkoeppe commented Jun 9, 2023

Thanks a lot for reviewing!

@vbraun vbraun merged commit 6252db6 into sagemath:develop Jun 21, 2023
11 of 13 checks passed
@mkoeppe mkoeppe added this to the sage-10.1 milestone 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.

None yet

3 participants