In OSCAR, polynomial rings should all be constructed with cached = false
#2455
Assignees
Comments
fingolfin
changed the title
cached = false
|
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. |
|
The overall situation is still rather bad, it wouldn't hurt to evangelize this again, and task some people to work on it. Running to find examples in Oscar.jl where code creates cached polynomial rings, one finds (excluding some obvious false hits):
|
|
@RafaelDavidMohr and I will take care of this. |
|
Since @ederc and @RafaelDavidMohr are working on this, triage had nothing to discuss |
|
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: 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 in julia |
|
Yes, I think this is what we want. Can you elaborate on the
Which tests are among the many tests? |
|
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
- destroy the algebra behaviour of the old instance
- magically behave and print weird.
…
--
Reply to this email directly or view it on GitHub:
#2455 (comment)
You are receiving this because you were mentioned.
Message ID: ***@***.***>
|
|
Well, let's say something like this from |
|
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:
#2455 (comment)
You are receiving this because you were mentioned.
Message ID: ***@***.***>
|
|
@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. |
|
Agreed. |
This was referenced Jun 10, 2024
|
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. |
|
Thank you @ederc |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
The text was updated successfully, but these errors were encountered: