Skip to content

Conversation

@fchapoton
Copy link
Contributor

@fchapoton fchapoton commented Sep 23, 2024

This removes the __len__ method in ring.pyx and let the category of infinite sets handle this.

The test with RR fails in some other bizarre way, so it is marked as a bug.

But maybe it would be better to just remove __len__ from rings instead ? People should use cardinality.

📝 Checklist

  • 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 and checked the documentation preview.

@github-actions
Copy link

github-actions bot commented Sep 23, 2024

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

Copy link
Collaborator

@tscrim tscrim left a comment

Choose a reason for hiding this comment

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

The issue with RR is that it is an extension class and __len__ is a special slot method in Cython. You can also see this failure with ZZ. So the thing to do is to change the __len__ implementation to be in parallel to Parent.__pow__.

@fchapoton
Copy link
Contributor Author

thanks a lot, Travis, for your continuous help and useful insights !

@tscrim
Copy link
Collaborator

tscrim commented Sep 25, 2024

No problem. Thank you for all your work on improving Sage!

@tscrim
Copy link
Collaborator

tscrim commented Oct 28, 2024

You still have the issue with RR, ZZ, etc. correct?

@mantepse
Copy link
Contributor

I agree that __len__ should not be a method in the category of rings. What is the reason for having it?

@fchapoton
Copy link
Contributor Author

Should I use "NotImplementedError: infinite set" or "ValueError: infinite set" ?
Currently, both do appear. It may be a good occasion to have more uniformity.

@tscrim
Copy link
Collaborator

tscrim commented Jul 16, 2025

I would say it would depend on if we expect the infinite set version to (eventually) work/be-implemented or not. Granted, we aren't consistent about this either... I would lean to ValueError as a general rule.

@fchapoton
Copy link
Contributor Author

So should I change the existing "NotImplementedError" ?

@tscrim
Copy link
Collaborator

tscrim commented Jul 16, 2025

If you want to, go ahead.

@fchapoton
Copy link
Contributor Author

Well, I finally would prefer not to do further changes here.

@fchapoton fchapoton marked this pull request as draft September 15, 2025 10:48
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.

3 participants