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

add some typing annotations to gens methods (-> tuple) #39233

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from

Conversation

fchapoton
Copy link
Contributor

adding some annotations inrings folder

to help find the cases where gens and related methods do not return a tuple (mostly for ideals now)

📝 Checklist

  • The title is concise and informative.
  • The description explains in detail what this PR is about.

Copy link

github-actions bot commented Dec 31, 2024

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

@@ -3919,10 +3918,12 @@ def __hash__(self):
return 0

@cached_method
def gens(self):
def gens(self): # -> PolynomialSequence
Copy link
Contributor

Choose a reason for hiding this comment

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

Same question here

Copy link
Contributor

Choose a reason for hiding this comment

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

At least here its relatively clear that it's returning PolynomialSequence_generic or not?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have used Sequence_generic here

@@ -622,7 +622,7 @@ def reduce(self, f):
"""
return f # default

def gens(self):
def gens(self): # -> tuple | PolynomialSequence
Copy link
Contributor

Choose a reason for hiding this comment

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

And here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

also not a class

Copy link
Contributor Author

Choose a reason for hiding this comment

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

and I did not make any serious analysis of the possible types that can appear here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This could use from sage.structure.sequence import Sequence_generic maybe. But I would rather keep it as it is now.

vbraun pushed a commit to vbraun/sage that referenced this pull request Jan 5, 2025
    
adding some annotations in`rings` folder

to help find the cases where `gens` and related methods do not return a
tuple (mostly for ideals now)

### 📝 Checklist

- [x] The title is concise and informative.
- [x] The description explains in detail what this PR is about.
    
URL: sagemath#39233
Reported by: Frédéric Chapoton
Reviewer(s): Frédéric Chapoton, Tobias Diez
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.

2 participants