Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Hide AbstractAlgebra ordering of a polynomial ring (a bit) #3374

Merged
merged 14 commits into from
Feb 21, 2024

Conversation

joschmitt
Copy link
Member

@joschmitt joschmitt commented Feb 15, 2024

The ordering keyword of polynomial_ring is renamed to internal_ordering to notify that this has nothing to do with the ordering keyword 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 and is a breaking change.

@@ -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)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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?

src/Rings/mpoly-ideals.jl Outdated Show resolved Hide resolved
test/Rings/groebner.jl Outdated Show resolved Hide resolved
@wdecker
Copy link
Collaborator

wdecker commented Feb 15, 2024 via email

@lgoettgens
Copy link
Member

I can provide you a patch to fix the find_name issues. It seems some new occurrences have been introduced after #3327.

@joschmitt
Copy link
Member Author

I can provide you a patch to fix the find_name issues. It seems some new occurrences have been introduced after #3327.

I assume that I just need to do PrettyPrinting.find_name?

@joschmitt
Copy link
Member Author

Otherwise be my guest.

@lgoettgens
Copy link
Member

No. The semantics have changed

@joschmitt
Copy link
Member Author

Please backport this. The change to ordering does not interfere with the book. I cannot vouch for the Hecke functions that are not exported any more unfortunately.

@joschmitt joschmitt added the backport 1.0.x Should be backported to the release 1.0 branch label Feb 21, 2024
@thofma thofma merged commit d32efdd into oscar-system:master Feb 21, 2024
20 checks passed
@joschmitt joschmitt deleted the js/ordering branch February 22, 2024 08:34
benlorenz pushed a commit that referenced this pull request Feb 22, 2024
* `ordering` -> `internal_ordering` for polynomial rings
* Adjust to unexported symbols from Hecke
* `find_name` now lives in `PrettyPrinting`

(cherry picked from commit d32efdd)
@benlorenz benlorenz mentioned this pull request Feb 22, 2024
33 tasks
benlorenz pushed a commit that referenced this pull request Feb 22, 2024
* `ordering` -> `internal_ordering` for polynomial rings
* Adjust to unexported symbols from Hecke
* `find_name` now lives in `PrettyPrinting`

(cherry picked from commit d32efdd)
@benlorenz benlorenz removed the backport 1.0.x Should be backported to the release 1.0 branch label Feb 22, 2024
benlorenz added a commit that referenced this pull request Feb 23, 2024
### 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
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Suggested change
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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Clarify how monomial orderings are supposed to be specified
9 participants