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

extended multi Hilbert series and other stuff #1220

Merged
merged 2 commits into from
Mar 31, 2022
Merged

extended multi Hilbert series and other stuff #1220

merged 2 commits into from
Mar 31, 2022

Conversation

wdecker
Copy link
Collaborator

@wdecker wdecker commented Mar 30, 2022

No description provided.

@wdecker
Copy link
Collaborator Author

wdecker commented Mar 30, 2022

@ederc Hi, could you please check the error message of the documentation test. I am not sure, what the problem is. On my machine there is no problem. Thx.

@ederc
Copy link
Member

ederc commented Mar 30, 2022

The problem is introduced in commit 2d1b1d78a8e9411ecfadabf267c005d43eb81ecb by @joschmitt : This commit introduces a default_ordering in groebner et al. calls depending on the ordering of the ring, whereas in the old way groebner worked we had as default degrevlex (in the global case). Your code first computes a leading ideal w.r.t. degrevlex (explicitly given in the parameter list) in multi_hilbert_function, but then the ideal membership test is done without explicitly giving an ordering, thus due to the above mentioned commit the default_ordering is the one of the ring which is in your case negdegrevlex, which is not a global ordering and thus the error occurs. Without the above commit the chosen default ordering would be degrevlex for which you have already a Groebner basis cached.

I assume the Oscar on your local machine is not up to date, so you do not run into this problem.

@wdecker
Copy link
Collaborator Author

wdecker commented Mar 30, 2022

Which ideal membership test do you mean? Once the ideal LI is computed, it is fixed. Where else is the choice of ordering relevant? I am not sure, whether I like the idea of the commit you mentioned. I missed this.

@ederc
Copy link
Member

ederc commented Mar 30, 2022

In multi_hilbert_function you later on ask FG(i) in LI. This in is ideal_membershipin mpoly-ideals.jl, which has now (due to the mentioned commit) no longer degrevlexbut default_ordering as default ordering. Thus groebner_assure tests w.r.t. default_ordering=negdegrevlex (from R) and not w.r.t. degrevlex w.r.t. which you have computed LI if we have already a cached GB.

As we discussed already some time ago: Of course we could rewrite ideal_membership to check if there is already a GB cached w.r.t. any monomial ordering and use this one, but this might be a bad choice in some situations.

@fieker fieker merged commit c5c95b7 into master Mar 31, 2022
@fieker fieker deleted the Wolfram branch March 31, 2022 06:52
@joschmitt
Copy link
Member

I'm happy to discuss default_ordering. I see now that local orderings are maybe not a good idea?
However, I still think that if a grading is given by positive weights on the variables, then the ordering, which is used if the user does not provide anything, should be wp with those weights.

@wdecker
Copy link
Collaborator Author

wdecker commented Mar 31, 2022

I first have to think about all the consequences. In particular, I have to think about all the functions which might or might not be affected by the changes (ideal_membership, homogenization etc.) For example, in theory, ideal_membership would now decide ideal membership in the localized polynomial ring in the case of a local ordering and no longer in the polynomial ring. So the help text is misleading. in Practise, ideal_membership seems to be broken now in the case of a local ordering. We have to check, why.

@joschmitt
Copy link
Member

Yes, I did not think about this, sorry. I think local orderings should not be used by default (except of course when the ring is the corresponding localization). I opened #1221 to fix this.

@wdecker
Copy link
Collaborator Author

wdecker commented Mar 31, 2022

@joschmitt
@doc Markdown.doc"""
minimal_generating_set(I::MPolyIdeal{<:MPolyElem_dec})
minimal_generating_set(I::MPolyQuoIdeal{<:MPolyElem_dec})

Given a homogeneous ideal $I$ in graded ring $R$ such that the grading gives rise
to a weighted monomial ordering, return an array containing a minimal set of
generators of $I$.
"""
function minimal_generating_set(I::MPolyIdeal{<:MPolyElem_dec}; ordering::MonomialOrdering = default_ordering(base_ring(I)))

I think this function should have some asserts: coefficient ring a field? is_z_graded? All weights positive? It needs to be checked, what is needed to make the Singular functionality work correctly. Also, the local case needs to be considered eventually.

@hannes14
Copy link
Member

needed asserts from the Singular side: coefficient are a field and (is_graded or local ordering)

@joschmitt
Copy link
Member

Regarding the local case: there is minimal_generators(::MPolyIdealLoc) which uses the Singular function minbase. With the name minimal_generating_set I followed the naming of Singular.jl.
Regarding checks: we can of course check whether the coefficient ring is a field and whether the ring is graded on the Oscar side. But in the end, the checks should be done in Singular.jl because if the ideal is not homogeneous w.r.t. the ordering (!) Singular will complain. That is to say, there should be a function minimal_generating_set in Singular.jl doing those sanity checks and calling minbase, which we can then use in Oscar; see also the discussion in #1191 .

@wdecker
Copy link
Collaborator Author

wdecker commented Mar 31, 2022

Should we then have the same name for the global and local case?

O.k. with what you said about Singular.jl

@joschmitt
Copy link
Member

Should we then have the same name for the global and local case?

I would say so. As I said, I followed the naming in Singular.jl, but I don't insist on either choice.

@wdecker
Copy link
Collaborator Author

wdecker commented Apr 3, 2022

@HechtiDerLachs Can you please rename minimal_generators(I::MPolyIdealLoc) to minimal_generating_set(I::MPolyIdealLoc)?

@HechtiDerLachs
Copy link
Collaborator

I'm afraid the MPolyIdealLoc is not due to me, but to Raul in src/Ring/mpoly-local.jl. In my opninion, this file is deprecated by now, but I didn't feel I had the authority to remove it.

@fieker
Copy link
Contributor

fieker commented Apr 4, 2022 via email

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

6 participants