Remove calls to PolyRing(R)

[fingolfin]

No description provided.

[codecov]

Codecov Report

Attention: Patch coverage is 55.55556% with 4 lines in your changes missing coverage. Please review.

Project coverage is 85.94%. Comparing base (65f9956) to head (62504fa).

Files	Patch %	Lines
src/generic/Misc/Poly.jl	0.00%	2 Missing ⚠️
src/Poly.jl	75.00%	1 Missing ⚠️
src/generic/Misc/Rings.jl	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1729      +/-   ##
==========================================
- Coverage   87.32%   85.94%   -1.39%     
==========================================
  Files         117      117              
  Lines       29768    29747      -21     
==========================================
- Hits        25996    25565     -431     
- Misses       3772     4182     +410     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[lgoettgens]

I am not a fan of using the not exported function polynomial_ring_only for this. The IMO preferred way would be R, _ = polynomial_ring(R, :x; cached=false), which would be consistent with other places where we do only want the first return value, e.g. residue_ring (which returns the residue ring and the epi)

[lgoettgens]

polynomial_ring_only seems to be more of an implementation detail that should not be relied on

[fingolfin]

I understand your points. But an annoying drawback of R, _ = polynomial_ring(R, :x; cached=false) is that it constructors a vector of polynomials and then discards them. That's a bunch of wasted allocations. E.g. in Oscar:

julia> F, _ = cyclotomic_field(10)
(Cyclotomic field of order 10, z_10)

julia> @btime AbstractAlgebra.polynomial_ring_only(F, [:x,:y]);
  297.506 ns (1 allocation: 64 bytes)

julia> @btime polynomial_ring(F, [:x,:y]);
  741.738 ns (17 allocations: 976 bytes)

[lgoettgens]

I understand your points. But an annoying drawback of R, _ = polynomial_ring(R, :x; cached=false) is that it constructors a vector of polynomials and then discards them.

But how is this different from many of the calls to polynomial_ring in Oscar? There are many more there that just discard the second return value. Maybe this is a sign of making polynomial_ring_only something less internal? But something like this should possibly be discussed in triage

[lgoettgens]

This signature is identical to the one 4 lines below (up to kwargs). And since julia ignores the kwargs for dispatch, it cannot decide between the two

[fingolfin]

I understand your points. But an annoying drawback of R, _ = polynomial_ring(R, :x; cached=false) is that it constructors a vector of polynomials and then discards them.

But how is this different from many of the calls to polynomial_ring in Oscar? There are many more there that just discard the second return value.

All of those are also bad (see also the now removed justification for the existence of PolyRing in https://github.com/Nemocas/Nemo.jl/pull/1790/files). But the fact that there is other code that is suboptimal is not a great justification for regressing / deoptimization code.

Maybe this is a sign of making polynomial_ring_only something less internal?

Maybe. I am not sure this is something we need to document for end users, though, but sure something for dev docs.

The name polynomial_ring_only is not great, though. Nor is PolyRing, as it doesn't fit our naming scheme; but I like how short it is. Perhaps we could rename it to poly_ring but deliberately keep it as not exported; but import it into Nemo/Hecke/Oscar, so that internal code can write poly_ring without qualifiers.

That would keep it separate from polynomial_ring_only, though, which is the central function to implement for custom polynomial ring implementations. But that's OK for now. (Thinking a step further, we could also change that: the main difference between the two is the default value for cached, which should be easy enough to adjust for. And we could go even further and think about removing the entire handling of caches from all the MPoly subtype implementations; instead implementing this in one central place. But that's getting us a bit far afield now)

But something like this should possibly be discussed in triage

Maybe.

[lgoettgens]

Thanks for working this idea out! I like the idea of having an internal poly_ring without caching (additionally to polynomial_ring_only). At least this seems like a local optimum to me aka something fine for now that is not too much work to put in.