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

signature for permutations has inconsistent signature compared to its behavior #36198

Open
2 tasks done
jhpalmieri opened this issue Sep 6, 2023 · 4 comments · May be fixed by #36199
Open
2 tasks done

signature for permutations has inconsistent signature compared to its behavior #36198

jhpalmieri opened this issue Sep 6, 2023 · 4 comments · May be fixed by #36199

Comments

@jhpalmieri
Copy link
Member

Steps To Reproduce

sage: s = Permutations(4)[10]
sage: type(s.signature())
<class 'int'>

as compared to

def signature(self) -> Integer:

in combinat/permutation.py

Expected Behavior

I think that signature should return an Integer, not an int. There may be similar issues in this file; for example

    def size(self) -> Integer:
        return len(self)

and a few others that are supposed to return type Integer but actually return len(something).

Actual Behavior

The method returns an int, not an Integer.

Additional Information

See https://stackoverflow.com/questions/77052185/unexpected-error-with-sign-function-in-sagemath/77054009#77054009 for one person running into problems because of this.

Environment

- **OS**: OS X 13.5.1
- **Sage Version**: 10.2.beta1

Checklist

  • I have searched the existing issues for a bug report that matches the one I want to file, without success.
  • I have read the documentation and troubleshoot guide
@dimpase
Copy link
Member

dimpase commented Sep 6, 2023

Yes, one need mypy, most probably.
E.g.

$ pip install mypy --user
$ cat t.py
def f() -> float:
        return "hello"
$ mypy t.py
t.py:2: error: Incompatible return value type (got "str", expected "float")  [return-value]
Found 1 error in 1 file (checked 1 source file)

No idea how it plays with cython, though.

Should we run mypy on all the *.py files, at least, in our CI?

@jhpalmieri
Copy link
Member Author

pyright was recommended, perhaps instead of mypy, on sage-devel. I don't have an opinion either way. Regardless, based on my understanding of the situation, I think it is appropriate that we are not currently recommending using type hints in the developer's guide. If we get good enough type-checking, we can change the documentation.

@jhpalmieri
Copy link
Member Author

I tried sage --pip install mypy, followed by sage --sh and then mypy src/sage/combinat/permutation.py. Maybe I need to invoke this differently?

$ mypy src/sage/combinat/permutation.py
src/sage/combinat/permutation.py:247: error: Skipping analyzing "sage.arith.misc": module is installed, but missing library stubs or py.typed marker  [import]
src/sage/combinat/permutation.py:248: error: Skipping analyzing "sage.categories.finite_enumerated_sets": module is installed, but missing library stubs or py.typed marker  [import]
src/sage/combinat/permutation.py:249: error: Skipping analyzing "sage.categories.finite_permutation_groups": module is installed, but missing library stubs or py.typed marker  [import]
...
src/sage/combinat/permutation.py:9676: error: Skipping analyzing "sage.misc.persist": module is installed, but missing library stubs or py.typed marker  [import]
Found 48 errors in 1 file (checked 1 source file)

jhpalmieri added a commit to jhpalmieri/sage that referenced this issue Sep 6, 2023
…hods

return type `Integer` instead of type `int`.
@fchapoton
Copy link
Contributor

I am using the bash alias

alias mypy='python3 -m mypy --follow-imports=skip --ignore-missing-imports --python-version=3.10'

vbraun pushed a commit to vbraun/sage that referenced this issue Mar 30, 2024
    
<!-- ^ Please provide a concise and informative title. -->
<!-- ^ Don't put issue numbers in the title, do this in the PR
description below. -->
<!-- ^ For example, instead of "Fixes sagemath#12345" use "Introduce new method
to calculate 1 + 2". -->
<!-- v Describe your changes below in detail. -->
<!-- v Why is this change required? What problem does it solve? -->
<!-- v If this PR resolves an open issue, please link to it here. For
example, "Fixes sagemath#12345". -->
Configuration from
sagemath#36198 (comment)
@fchapoton @jhpalmieri


### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->

- [x] The title is concise and informative.
- [ ] 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

<!-- List all open PRs that this PR logically depends on. For example,
-->
<!-- - sagemath#12345: short description why this is a dependency -->
<!-- - sagemath#34567: ... -->
    
URL: sagemath#37522
Reported by: Matthias Köppe
Reviewer(s): Frédéric Chapoton
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants