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

Add more monomial and module orderings #723

Merged
merged 38 commits into from
Jan 29, 2022

Conversation

wbhart
Copy link
Contributor

@wbhart wbhart commented Oct 7, 2021

  • Module orderings (incl. TOP and POT defined via * operator)
  • Weighted orderings defined by weight vector
  • Numerous missing orderings
  • Factored out orderings into orderings.jl

@wdecker
Copy link
Collaborator

wdecker commented Oct 8, 2021

I have two questions here:

  1. wnegdeglex, wnegdegrevlex: shouldn't we rather write negwdeglex, negwdegrevlex,
    keeping the wdeg together?

  2. How should, for example, deglex(::AbstractVector{<:MPolyElem}) show up when calling the groebner_basis function?

Currently, this reads

groebner_basis(I::MPolyIdeal; ordering::Symbol = :degrevlex, complete_reduction::Bool = false)

Should we write

groebner_basis(I::MPolyIdeal; ordering=:degrevlex, complete_reduction::Bool = false)

and something like

groebner_basis(I::MPolyIdeal; ordering=:wdegrevlex(W::Vector{Int}), complete_reduction::Bool = false)

in the weighted case?

Opinions?

@wbhart
Copy link
Contributor Author

wbhart commented Oct 8, 2021

We currently haven't dealt with the std function, and will need to change the signature to support orderings that are not given by symbols.

I have no opinion on renaming the orderings. I thought that we followed the decision that was made at the meeting, but on second thought perhaps we didn't discuss this part in sufficient detail.

@HechtiDerLachs
Copy link
Collaborator

HechtiDerLachs commented Oct 19, 2021

Has any further progress been made so that the groebner_basis function can digest the new orderings which are not merely given by symbols?

Edit: I found that the singular_ring routine can be used with the new orderings. But when p is of type MonomialOrdering, then one has to call singular_ring(R, p.o) rather than singular_ring(R, p), i.e. one has to refer to an inner field of the actual ordering as an argument. This is not how it's supposed to be, right?

Did I miss out on the intended way to use this, or is this going to be addressed in the future?

@tthsqe12
Copy link
Contributor

Access to the private fields shouldn't be the part of any publich interface (unless it is explicitly documented).

@wbhart
Copy link
Contributor Author

wbhart commented Oct 19, 2021

We haven't done that yet. We are waiting on some things downstream, but they are well in hand.

@wbhart wbhart force-pushed the ordering_gb_meeting branch 2 times, most recently from f581579 to 5c40c09 Compare October 19, 2021 13:30
@fingolfin
Copy link
Member

What's the status of this? Briefly, please: you said you are waiting for some things downstream. Which downstream is meant? AA? or Singular? It would be good to track this here so we know who is waiting for whom right now

@wbhart
Copy link
Contributor Author

wbhart commented Oct 28, 2021

At present that has been worked around. We were waiting for block_diagonal matrices in AbstractAlgebra, but it turns out Hecke had them under a different name.

The main thing preventing merge here is documentation, which is today's task.

@wbhart
Copy link
Contributor Author

wbhart commented Oct 28, 2021

Actually, I take it back. It seems that all the orderings need to be implemented on the Oscar side so that we can test them. The only reasonable test would be to check that a list of terms with respect to all these orderings end up in the right order.

As we don't maintain the Singular polynomials as well (or even have a conversion function as far as I can see), this means the orderings need to be implemented on the Oscar side, and this appears to be somewhat nontrivial (some of the basic ones are already there). Also, presumably some of the existing code actually depends on this working...

On the other hand, writing code to test on the Oscar side hardly tests that we hooked it all up to Singular correctly. So we also need tests of some Singular functionality that tests the orderings such as GB and GB_with_transform.

@hannes14 said he will also provide some test cases for the GB and leading_ideal stuff we added, tomorrow.

So, it's going to be next week before all this is done.

@wbhart
Copy link
Contributor Author

wbhart commented Oct 29, 2021

Current status: we fixed the problems we were aware of.

I also want to add the ability to do monomial comparison using the new orderings (on the Oscar side) and tests for this. This will be a priority next week.

@wbhart
Copy link
Contributor Author

wbhart commented Oct 29, 2021

I am also unsure if leading_term/monomial/coefficient are working with the new orderings. I need to check this and possibly add more tests, etc.

@tthsqe12
Copy link
Contributor

So, what is the status here? I see a test is failing with :negwdegrevlex with weight vector [1,2,1] on variables [x,y,z]
From what I can see in the code, this constructs the weight matrix [-1 -2 -1; -1 0 0; 0 -1 0]. With the monomials x^0*y^0*z^3 and x^1*y^1*z^0, I get a tie in the first row but the second row says the first monomial x^0*y^0*z^3 is bigger. So, the just the test needs to be fixed?

@hannes14
Copy link
Member

No, x*y should be larger than z3, the weight matrix should be [-1 -2 -1;0 0 -1; 0 -1 0].
I am working on it.

@fingolfin
Copy link
Member

What's the status of this?

@wbhart
Copy link
Contributor Author

wbhart commented Dec 2, 2021

The monomial orderings simply need to be corrected by @hannes14 . The branch belongs to him currently.

However, I believe he's been busy working on the Singular release, so we have to wait until he gets back to it.

@thofma
Copy link
Collaborator

thofma commented Dec 16, 2021

Someone recently asked me about this. Is there an update on this? @hannes14?

@fingolfin
Copy link
Member

Apparently there is a conflict here and @hannes14 and @fieker are supposed to look into resolving this

@fingolfin
Copy link
Member

fingolfin commented Jan 18, 2022

I've merged master into this now.

I hope this can now proceed?

The tested removed in test/Rings/mpoly-test.jl are now in
test/Rings/orderings-test.jl
@oscar-system oscar-system deleted a comment from wbhart Jan 18, 2022
@oscar-system oscar-system deleted a comment from fingolfin Jan 18, 2022
@tthsqe12
Copy link
Contributor

So, I am looking into the merge conflicts right now. They do not look too bad, but I hope I haven't spoken too soon.

@tthsqe12
Copy link
Contributor

this should be good to go now

@tthsqe12
Copy link
Contributor

I can only guess the following was happening with the orderings:
the name "degrevlex" suggests it is "deg" + "revlex" but in fact it is "deg" plus "negrevlex". This combined with all of the possible permutations understandably had us confused.

@thofma
Copy link
Collaborator

thofma commented Jan 28, 2022

Docstrings need adjustment.

@tthsqe12 tthsqe12 changed the title [WIP] Add more monomial and module orderings Add more monomial and module orderings Jan 28, 2022
@tthsqe12
Copy link
Contributor

merge me please

@thofma thofma merged commit 4d42596 into oscar-system:master Jan 29, 2022
bschroter pushed a commit to bschroter/oscar-matroids that referenced this pull request Feb 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants