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

using Parent in integer rings instead of ParentWithGens #37156

Merged
merged 1 commit into from
Feb 2, 2024

Conversation

fchapoton
Copy link
Contributor

@fchapoton fchapoton commented Jan 24, 2024

Let's try to get rid of the last direct inheritance to ParentWithGens.

(not quite the last one, see #37158 )

There remains many indirect inheritance, mostly through Ring, Algebra, CommutativeRing and their subclasses. This is a step towards cleaning and simplifying the old coercion framework.

The present changes is close to the heart of sage, so appropriate timings should be checked.

📝 Checklist

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

Copy link

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

@kwankyu
Copy link
Collaborator

kwankyu commented Jan 29, 2024

The present changes is close to the heart of sage, so appropriate timings should be checked.

What timings are necessary? Would you be explicit? Did you do the timings?

@tscrim
Copy link
Collaborator

tscrim commented Jan 31, 2024

I think coercions and conversions with common objects (finite fields, constant polynomials, rational numbers, etc.) would be good. Although I wouldn't expect really any changes in the timings...

@fchapoton
Copy link
Contributor Author

I ran the tests of the rings folder and the time taken seemed not to change significantly. Not done scientifically..

@kwankyu
Copy link
Collaborator

kwankyu commented Jan 31, 2024

I got, where m is a 1000*1000 matrix of integer entries,

Before this PR:

sage: m = load('m')
sage: timeit('m^3+m^2+m')
5 loops, best of 3: 1.14 s per loop
sage: timeit('1/3*m')
5 loops, best of 3: 447 ms per loop

After this PR:

sage: m = load('m')
sage: timeit('m^3+m^2+m')
5 loops, best of 3: 1.14 s per loop
sage: timeit('1/3*m')
5 loops, best of 3: 444 ms per loop

Perhaps not enough scientific either, but I don't perceive any performance regression.

Copy link
Collaborator

@kwankyu kwankyu left a comment

Choose a reason for hiding this comment

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

LGTM

@vbraun vbraun merged commit e3f7c7f into sagemath:develop Feb 2, 2024
21 of 22 checks passed
@fchapoton fchapoton deleted the parent_for_integer_ring branch February 3, 2024 08:18
@mkoeppe mkoeppe added this to the sage-10.3 milestone Mar 7, 2024
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.

5 participants