search

oscar-system / Oscar.jl

A comprehensive open source computer algebra system for computations in algebra, geometry, and number theory.
https://www.oscar-system.org
Other
345 stars 126 forks source link

In OSCAR, polynomial rings should all be constructed with `cached = false` #2455

Open HereAround opened 1 year ago

HereAround commented 1 year ago

This was brought to my attention by @fieker. Apparently, this happens in particular for schemes. Hence, I cc @HechtiDerLachs , so we can discuss this at some point.

fingolfin commented 1 year ago

There is some relevant text on the caches in the AA documentation, as part of the ring interface. We should check if this text still reflects what we want, and then copy it (after editing) into the OSCAR dev docs.

Also we need to think about allowing optional specification of parents in more functions, e.g. hilbert_series; there is an open PR #2084 by @HechtiDerLachs on this, which we need to get finished and merged.

fingolfin commented 6 months ago

The overall situation is still rather bad, it wouldn't hurt to evangelize this again, and task some people to work on it.

Running

git grep -n '\bpolynomial_ring(' src | fgrep -v 'julia> ' | fgrep -v 'src/InvariantTheory' | egrep -v 'cached\s*=\s*false'

to find examples in Oscar.jl where code creates cached polynomial rings, one finds (excluding some obvious false hits):

ederc commented 6 months ago

@RafaelDavidMohr and I will take care of this.

lgoettgens commented 6 months ago

Since @ederc and @RafaelDavidMohr are working on this, triage had nothing to discuss

ederc commented 5 months ago

I looked a bit more into this issue, but to tell you the truth, I may not understand completely what to do. At the moment we have this behaviour:

julia> R, (x,y) = polynomial_ring(QQ, ["x", "y"]; cached = false)
(Multivariate polynomial ring in 2 variables over QQ, QQMPolyRingElem[x, y])

julia> S, (x,y) = polynomial_ring(QQ, ["x", "y"]; cached = false)
(Multivariate polynomial ring in 2 variables over QQ, QQMPolyRingElem[x, y])

julia> R == S
false

So this means that many tests will break if we are not caching internally constructed rings since the objects we return are usually connected with those rings. Looking at the code checking equality of non-cached rings R == S it boils down to 

==(x, y) = x === y

in julia Base.jl. Is this really what we want?

thofma commented 5 months ago

Yes, I think this is what we want. Can you elaborate on the

So this means that many tests will break if we are not caching internally constructed rings since the objects we return are usually connected with those rings.

Which tests are among the many tests?

fieker commented 5 months ago

On Thu, Jun 06, 2024 at 12:25:25AM -0700, ederc wrote:

I looked a bit more into this issue, but to tell you the truth, I may not understand completely what to do. At the moment we have this behaviour:

julia> R, (x,y) = polynomial_ring(QQ, ["x", "y"]; cached = false)
(Multivariate polynomial ring in 2 variables over QQ, QQMPolyRingElem[x, y])

julia> S, (x,y) = polynomial_ring(QQ, ["x", "y"]; cached = false)
(Multivariate polynomial ring in 2 variables over QQ, QQMPolyRingElem[x, y])

julia> R == S
false

So this means that many tests will break if we are not caching internally constructed rings since the objects we return are usually connected with those rings. Looking at the code checking equality of non-cached rings R == S it boils down to 

==(x, y) = x === y

in julia Base.jl. Is this really what we want? Yes.

The problem is, conceptually: here you set yourself up to expect true: you deliberately create the ring twice, immediately after each other.

Now consider a different scenario: you compute a normalisation, which will return a "new" polynomial ring and you compute a, don't know, blow-up that also creates a polynomial ring. Would you be happy to have the 2 rings be compatibel if the programmers randomly choose to call the variables x and y?

To make things worse: suppose a ring is created to represent some fin. gen. algebra. As such, the special-printing code is used to reflect that. Now, by chance, you create a ring with the same variables. If the ring is the "same", it will either

-- Reply to this email directly or view it on GitHub: https://github.com/oscar-system/Oscar.jl/issues/2455#issuecomment-2151583977 You are receiving this because you were mentioned.

Message ID: @.***>

ederc commented 5 months ago

Well, let's say something like this from AffineRationalPoint.jl:

  A2 = affine_space(GF(2), [:x, :y]);
  (x, y) = coordinates(A2);
  X = algebraic_set(x*y);

  pA = A2([1,0])

  A2a = affine_space(GF(2), [:x, :y]);
  @test A2a([1,0]) == pA
fieker commented 5 months ago

On Thu, Jun 06, 2024 at 12:41:25AM -0700, ederc wrote:

Well, let's say something like this from AffineRationalPoint.jl:

  A2 = affine_space(GF(2), [:x, :y]);
  (x, y) = coordinates(A2);
  X = algebraic_set(x*y);

  pA = A2([1,0])

  A2a = affine_space(GF(2), [:x, :y]);
  @test A2a([1,0]) == pA

But the "same" affine space can (will be) a different patch of some projective object? And as such should be different? -- Reply to this email directly or view it on GitHub: https://github.com/oscar-system/Oscar.jl/issues/2455#issuecomment-2151611208 You are receiving this because you were mentioned.

Message ID: @.***>

ederc commented 5 months ago

@fieker @thofma I can understand the behaviour of non-cached rings, the problem is just that then this issue is nothing @RafaelDavidMohr and I can solve on our own globally. Then everybody has to take care of her/his own code. I don't think it is a good idea if @RafaelDavidMohr and I change tests for code where we do not have expertise.

thofma commented 5 months ago

Agreed.

ederc commented 4 months ago

Once the referenced PRs are all merged, this issue can be closed. As discussed, the open PRs need adjustments in tests. I asked the corresponding authors for support.

fingolfin commented 4 months ago

Thank you @ederc