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

Various small improvements to imports #39276

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

Conversation

tobiasdiez
Copy link
Contributor

Mostly to try to reduce a few circular imports that happen when you don't import sage.all. A few of these changes are purely cosmetic.

📝 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.

⌛ Dependencies

Mostly to try to reduce a few circular imports that happen when you don't import `sage.all`. A few of these changes are purely cosmetic.
Copy link

github-actions bot commented Jan 5, 2025

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

Copy link
Contributor

@user202729 user202729 left a comment

Choose a reason for hiding this comment

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

Maybe add some tests that the circular imports are fixed?

There's test_executable convenience function.

@@ -182,7 +181,7 @@ cdef Rational_sub_(Rational self, Rational other):

return x

cdef Parent the_rational_ring = sage.rings.rational_field.Q
cdef Parent the_rational_ring = Q
Copy link
Contributor

Choose a reason for hiding this comment

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

Not strictly related but maybe we can change to use QQ instead

Copy link
Contributor

Choose a reason for hiding this comment

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

and may be use from sage.rings.rational_field import Q as the_rational_ring ?

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