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

PR for #36198: fix return types in permutation.py so that methods return type Integer instead of type int. #36199

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

jhpalmieri
Copy link
Member

For methods in combinat/permutation.py which have return type hint of Integer, actually return an Integer rather than an int.

This should fix #36198.

📝 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

…hods

return type `Integer` instead of type `int`.
@jhpalmieri
Copy link
Member Author

We could add some doctests but I have not done that.

@github-actions
Copy link

github-actions bot commented Sep 6, 2023

Documentation preview for this PR (built with commit de93962; changes) is ready! 🎉

@tobiasdiez
Copy link
Contributor

Does it not make more sense to fix the type annotation to use int?

@jhpalmieri
Copy link
Member Author

(a) I think whoever added the type hints had a reason for preferring Integer. (b) Some of the methods already return type Integer, and it makes some sense to be consistent among them. (c) The Sage Integer type is more powerful than int and has more methods attached to it. I find it surprising when a Sage method returns int rather than Integer.

@jhpalmieri
Copy link
Member Author

And (d), the motivating problem arose because it was returning int, not Integer.

@tobiasdiez
Copy link
Contributor

For a) let's ask @fchapoton about his intentions (d7a9ea8). The other things are good points, although personally having size and length return a sage-integer feels overkill.

@fchapoton
Copy link
Contributor

I have no precise intention, and no preference between "int" and "Integer". In my opinion, the type should be an abstract version of the integers, not any specific implementation. Alas, there is nothing like that readily available, as far as I can tell.

So if I wrote "-> Integer", the intended meaning was "return an integer in NN" and not "return a Sage integer".

@tscrim
Copy link
Collaborator

tscrim commented Sep 7, 2023

In general, as @jhpalmieri said, Sage should return Sage integers as the behavior is better (and this is consistent with what we do elsewhere in Sage). If it returns a Python int, it is generally a bug IMO.

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.

signature for permutations has inconsistent signature compared to its behavior
5 participants