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

Check input of composition #38366

Merged
merged 3 commits into from
Jul 24, 2024
Merged

Conversation

mantepse
Copy link
Collaborator

Fix #14862

Copy link

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

@mantepse
Copy link
Collaborator Author

@fchapoton, I think that this is an improvement, so let's get it in.

In the longer run I'd really like to make Composition only accept positive integers, and use IntegerVector for weak compositions (perhaps WeakComposition should be an alias). However, this can be done incrementally, I am currently doing it for tableau.py.

Copy link
Collaborator Author

@mantepse mantepse left a comment

Choose a reason for hiding this comment

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

Looks good to me. Perhaps we should have a check argument to bypass the checks, but I don't think it is necessary right now.

@mantepse
Copy link
Collaborator Author

@fchapoton, I just realised that I cannot approve this formally, because I created the pull request...

@fchapoton
Copy link
Contributor

I think it would be good to have Travis' opinion on that. @tscrim what do you think ?

@tscrim
Copy link
Collaborator

tscrim commented Jul 15, 2024

I think it would be good to add a check to the __init__, but I wouldn't make a requirement for setting this to a positive review.

@vbraun vbraun merged commit c8fd3dc into sagemath:develop Jul 24, 2024
22 checks passed
@mkoeppe mkoeppe added this to the sage-10.5 milestone Jul 25, 2024
@fchapoton fchapoton deleted the input_of_composition branch July 27, 2024 15:27
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.

Compositions accept any input
5 participants