-
Notifications
You must be signed in to change notification settings - Fork 71
Remove redundancy in UniversalPolyRing
#2176
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
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #2176 +/- ##
=======================================
Coverage 87.92% 87.93%
=======================================
Files 127 127
Lines 31780 31792 +12
=======================================
+ Hits 27943 27955 +12
Misses 3837 3837 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Really nice, thank you! Just one minor quirk mentioned above |
|
Before this PR (and for the julia> R,(x,y) = universal_polynomial_ring(QQ, [:x, :y]; cached=false);
julia> @b gen(R, :x)
124.765 ns (16 allocs: 608 bytes)
julia> @b copy(x)
46.943 ns (6 allocs: 256 bytes)With this PR: julia> R,(x,y) = universal_polynomial_ring(QQ, [:x, :y]; cached=false);
julia> @b gen(R, :x)
140.961 ns (17 allocs: 640 bytes)So this PR still allocates more and is a tad slower |
| idx = Int[] | ||
| current_symbols = symbols(S) | ||
| n = length(current_symbols) | ||
| added_symbols = Symbol[] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is probably the extra allocation. But also idx is a redundant allocation when we are in the gen case.
All in all I think my version which added a separate gen method is still desirable.
|
|
||
| base_ring_type(::Type{<:UniversalPolyRing{T}}) where T = parent_type(T) | ||
| base_ring(S::UniversalPolyRing) = S.base_ring::base_ring_type(S) | ||
| base_ring(S::UniversalPolyRing) = base_ring(mpoly_ring(S))::base_ring_type(S) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In hindsight this definition of base_ring was wrong: it should have been
| base_ring(S::UniversalPolyRing) = base_ring(mpoly_ring(S))::base_ring_type(S) | |
| base_ring(S::UniversalPolyRing) = mpoly_ring(S))::base_ring_type(S) |
(obviously with base_ring_type changed as well), and then coefficient_ring(S) would return base_ring(base_ring(S)).
That would then also fit naturally with the new UniversalRing{T} type: anything that uses mpoly_ring now would simply use base_ring.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thinking about this some more, I would suggest we actually really change this for UniversalRing -- then it would be a breaking change (assuming that we'd replace UniversalPolyRing{T} by UniversalRing{mpoly_type(T)}), but this would only affect GenericCharacterTables.jl (at least in the known OSCAR-verse -- someone else might rely on it, but in the release notes for the "breaking" item, we'd just suggest that affected code should use coefficient_ring instead of base_ring.
(And we'd keep providing mpoly_ring, but only as mpoly_ring(S::UniversalRing{<:MPolyRingElem}))
All of this is not for this PR, though
In preparation for #2172 I'm trying to simplify the implementation of universal polynomial rings.