Hide AbstractAlgebra ordering of a polynomial ring (a bit)
#3374
Conversation
| @@ -25,15 +25,13 @@ of the coefficient ring of the polynomial ring. | |||
| The basic constructor below allows one to build multivariate polynomial rings: | |||
|
|
|||
| ```@julia | |||
| polynomial_ring(C::Ring, V::Vector{String}; ordering=:lex, cached::Bool = true) | |||
| polynomial_ring(C::Ring, V::Vector{String}; cached::Bool = true) | |||
There was a problem hiding this comment.
@wdecker I removed mentioning this keyword here and below in this file. I think this way, it is less confusing for the non-expert user who does not need to know about such implementation details (it is still explained in the documentation on rings which we copy from AbstractAlgebra). Is this fine?
|
I agree, thanks for doing this, WolframVon meinem iPhone gesendetAm 15.02.2024 um 15:53 schrieb Johannes Schmitt ***@***.***>:
@joschmitt commented on this pull request.
In src/Rings/mpoly-ideals.jl:
@@ -1944,14 +1944,13 @@ julia> grassmann_pluecker_ideal(2, 4)
Ideal generated by
x[1]*x[6] - x[2]*x[5] + x[3]*x[4]
…-julia> R, x = polynomial_ring(residue_ring(ZZ, 7)[1], "x" => (1:2, 1:3), ordering=:degrevlex)
+julia> R, x = polynomial_ring(residue_ring(ZZ, 7)[1], "x" => (1:2, 1:3))
Okay, then we should really do this (override the ring). I don't think we can or should expect from the user to supply a ring with the correct ordering. I'll look into it.
—Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you were mentioned.Message ID: ***@***.***>
|
joschmitt
force-pushed
the
js/ordering
branch
from
b282a6e
to
be1b133
Compare
joschmitt
force-pushed
the
js/ordering
branch
from
be1b133
to
3690653
Compare
lgoettgens
added
package: Hecke
package: Nemo
package: AbstractAlgebra
and removed
needs hecke update
needs nemo update
labels
|
I can provide you a patch to fix the |
I assume that I just need to do |
|
Otherwise be my guest. |
|
No. The semantics have changed |
|
Please backport this. The change to |
joschmitt
added
the
backport 1.0.x
* `ordering` -> `internal_ordering` for polynomial rings * Adjust to unexported symbols from Hecke * `find_name` now lives in `PrettyPrinting` (cherry picked from commit d32efdd)
* `ordering` -> `internal_ordering` for polynomial rings * Adjust to unexported symbols from Hecke * `find_name` now lives in `PrettyPrinting` (cherry picked from commit d32efdd)
benlorenz
removed
the
backport 1.0.x
### Backported PRs - [x] #3367 - [x] #3379 - [x] #3369 - [x] #3291 - [x] #3325 - [x] #3350 - [x] #3351 - [x] #3365 - [x] #3366 - [x] #3382 - [x] #3373 - [x] #3341 - [x] #3346 - [x] #3381 - [x] #3385 - [x] #3387 - [x] #3398 - [x] #3399 - [x] #3374 - [x] #3406 - [x] #2823 - [x] #3298 - [x] #3386 - [x] #3412 - [x] #3392 - [x] #3415 - [x] #3394 - [x] #3391
| @@ -311,7 +311,7 @@ function singular_generators(B::IdealGens, monorder::MonomialOrdering=default_or | |||
| singular_assure(B) | |||
| # in case of quotient rings, monomial ordering is ignored so far in singular_poly_ring | |||
| isa(B.gens.Ox, MPolyQuoRing) && return B.gens.S | |||
| isdefined(B, :ord) && B.ord == monorder && monomial_ordering(B.Ox, ordering(base_ring(B.S))) == B.ord && return B.gens.S | |||
| isdefined(B, :ord) && B.ord == monorder && monomial_ordering(B.Ox, Singular.ordering(base_ring(B.S))) == B.ord && return B.gens.S | |||
There was a problem hiding this comment.
@joschmitt : This change leads to a use of 30% of ~4.3s computation time in an example of @simonbrandhorst and mine. The issue seems to be that a new monomial ordering is created and then comparison can not rely on caching. Can we find a workaround for this?
There was a problem hiding this comment.
I don't see how this change can have a performance impact? The only thing happening in this line should be that ordering is not exported from Singular anymore. One could also do
| isdefined(B, :ord) && B.ord == monorder && monomial_ordering(B.Ox, Singular.ordering(base_ring(B.S))) == B.ord && return B.gens.S | |
| isdefined(B, :ord) && B.ord == monorder && monomial_ordering(B.Ox, internal_ordering(base_ring(B.S))) == B.ord && return B.gens.S |
I think. I'm fairly certain that all I did was renaming a function.
There was a problem hiding this comment.
@HechtiDerLachs just to clarify: when you say this change causes the slow down, how did you determine this, resp. check this? By reverting just that one change and then benchmarking again?
If so, knowing the example (or a minimized version) would be essential to figure out what's going on.
There was a problem hiding this comment.
I have to admit that I did not revert this particular change to see whether it solves the problem. Sorry about this premature notification, @joschmitt . Either way: There seems to be an issue with the comparison of orderings here again.
A small example will be hard to construct. But I have one on my machine right now and I'm in my office.
The
orderingkeyword ofpolynomial_ringis renamed tointernal_orderingto notify that this has nothing to do with theorderingkeyword of many other functions in OSCAR and to discourage non-expert users from using it.I removed all mentions of this keyword from the native OSCAR documentation. It is still mentioned/explained in the doc string coming from AbstractAlgebra which I adjusted a bit in Nemocas/AbstractAlgebra.jl#1597. I think lengthy explanations "why one shouldn't use it" are only confusing.
The keyword is used in more places than I expected. I ping the respective authors (according to
git blame) in the comments with questions.Closes #3040.
Requires Nemocas/AbstractAlgebra.jl#1597, Nemocas/Nemo.jl#1678, thofma/Hecke.jl#1402, oscar-system/Singular.jl#783, algebraic-solving/AlgebraicSolving.jl#43 andis a breaking change.