Sets poly ring caching to false for modules

[ederc]

See #2455.

Again, some tests are failing due to different parent rings, thus I would like @jankoboehm and @HechtiDerLachs to have a look.

[HechtiDerLachs]

I think this should fix the tests.

But we should discuss whether or not we really want this behavior. Maybe it's reasonable to cache the ring for the Hilbert series of all modules in the base_ring so that we do not get different parents for different modules over the same ring? But this is just a suggestion. I do not have a strong personal opinion on this.

@wdecker , @jankoboehm .

[codecov]

Codecov Report

Attention: Patch coverage is 89.47368% with 2 lines in your changes missing coverage. Please review.

Project coverage is 84.07%. Comparing base (edb44e2) to head (4d0e278).

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #3872      +/-   ##
==========================================
+ Coverage   84.05%   84.07%   +0.02%     
==========================================
  Files         592      592              
  Lines       81746    81757      +11     
==========================================
+ Hits        68708    68738      +30     
+ Misses      13038    13019      -19     

Files	Coverage Δ
src/Rings/mpoly-graded.jl	91.36% <ø> (ø)
src/Modules/hilbert.jl	81.94% <89.47%> (+1.64%)	⬆️

... and 5 files with indirect coverage changes

[ederc]

Thanks for fixing.

[HechtiDerLachs]

The last commit is a suggestion for how we can regain usability after briefly consulting with @wdecker . Let me know, everyone, in case there is further need for discussion.

CC: @JohnAAbbott

[fieker]

On Mon, Jul 01, 2024 at 04:28:34AM -0700, Matthias Zach wrote: I think this should fix the tests. But we should consider whether or not we really want this behavior. Maybe it's reasonable to cache the ring for the Hilbert series of all modules in the `base_ring` so that we do not get different parents for different modules over the same ring?

There is also Hecke.Globals.Qx and Hecke.Globals.Zx that can potentially be used...

…

@wdecker , @jankoboehm . -- Reply to this email directly or view it on GitHub: #3872 (comment) You are receiving this because you are subscribed to this thread. Message ID: ***@***.***>

[fingolfin]

How about always using a multivariate polynomial ring?

Suggested change

	S.hilbert_series_parent = laurent_polynomial_ring(ZZ, :t; cached=false)[1]
	S.hilbert_series_parent = laurent_polynomial_ring(ZZ, [:t]; cached=false)[1]

That would make the return value of this function potentially type stable (well, with some further changes). As @jankoboehm says "this might also help with some other issues". But of course it might also cause issues... So this is really an open question.

[HechtiDerLachs]

Unless people come up with qualified opinions on this, I will leave it as is since these were the rings used in the source code up to this point for the different cases.

Type stability did not seem to matter before. If it matters now, I need clear advice which ring to use.

[joschmitt]

Doctests fail?

[lgoettgens]

Evaluated output:
│ 
│ (-t^2 + 1)^1*(-t^4 + 1)^1*(-t^3 + 1)^1
│ 
│ Expected output:
│ 
│ (-t^4 + 1)^1*(-t^3 + 1)^1*(-t^2 + 1)^1

to my eyes, these look identical

[ederc]

Yes, but I do not completely understand why the order of the terms change when not caching the rings when computing the hilbert series.

[JohnAAbbott]

Maybe the order of the terms/factors was never deterministic, and just by chance we see it now? It may be coincidence that it arises after changing the caching?

[joschmitt]

Somebody put effort in it that the denominator is printed in this factorized form. I agree that it is mathematically equivalent, but whoever made this printing might have an opinion on the order of exponents as well.

[fieker]

On Thu, Jul 04, 2024 at 06:35:15AM -0700, JohnAAbbott wrote: Maybe the order of the terms/factors was never deterministic, and just by chance we see it now? It may be coincidence that it arises after changing the caching?

This is a dict internally, the order of keys in a dict is non-deterministic (from a users point of view)

…

-- Reply to this email directly or view it on GitHub: #3872 (comment) You are receiving this because you commented. Message ID: ***@***.***>

[joschmitt]

It appears that the doc test fails because previously multi_hilbert_series would return elements of a multivariate Laurent polynomial ring (which might actually only have one variable). This ring is created with _hilbert_series_ring which is now partially duplicated by the new hilbert_series_parent.

[joschmitt]

I pushed a commit so that different parents are used for hilbert_series and multi_hilbert_series. This should also restore type stability.

[ederc]

@joschmitt Thanks, looks good to me.

[HechtiDerLachs]

This also looks like a reasonable solution to me. Thanks @joschmitt . Maybe @JohnAAbbott can have a look and confirm that everything is alright with his part of the code on Hilbert series?

[JohnAAbbott]

The changes look good to me -- but is the type specification on line 126 of src/Modules/hilbert.jl correct? Can it not be simply parent::Ring = ...?

[joschmitt]

The changes look good to me -- but is the type specification on line 126 of src/Modules/hilbert.jl correct? Can it not be simply parent::Ring = ...?

Thank you, you are right! I corrected it.

[joschmitt]

Approved by @JohnAAbbott (and also the triage meeting last week).