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

homogenization for ideal with ZZ^m grading #2372

Merged
merged 34 commits into from Jul 27, 2023

Conversation

JohnAAbbott
Copy link
Contributor

Homogenization for ideals in ZZ^m-graded ring.
Also trick to make it a bit faster (sometimes).

@codecov
Copy link

codecov bot commented May 11, 2023

Codecov Report

Merging #2372 (083490b) into master (c9b9a80) will decrease coverage by 0.55%.
The diff coverage is 15.54%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2372      +/-   ##
==========================================
- Coverage   72.66%   72.11%   -0.55%     
==========================================
  Files         429      429              
  Lines       60036    60501     +465     
==========================================
+ Hits        43623    43633      +10     
- Misses      16413    16868     +455     
Files Changed Coverage Δ
src/Rings/mpoly-graded.jl 73.76% <14.38%> (-8.82%) ⬇️
experimental/PlaneCurve/src/AffinePlaneCurve.jl 88.82% <100.00%> (ø)

... and 22 files with indirect coverage changes

@wdecker
Copy link
Collaborator

wdecker commented May 13, 2023

Do not remove this line as it has not yet been addressed:

TODO: Adjust for ZZ-gradings as soon as weighted orderings are available

Comment on lines 2219 to 2221
if is_zero(I) || is_one(I)
return I
end
Copy link
Collaborator

Choose a reason for hiding this comment

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

aaa

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sorry, what I mean here is that you should not returnI as you have to return an ideal in a new polynomial ring with homogenizing variables added.

Comment on lines 2211 to 2215
GB1 = elements(groebner_basis(I, ordering=degrevlex(OrigR)))
GB1 = filter(f -> (num_terms(f) < 2*AveNumTerms), GB1)
GB2 = elements(groebner_basis(I, ordering=deglex(OrigR))) # or degrevlex with vars in reverse order?
GB2 = filter(f -> (num_terms(f) < 2*AveNumTerms), GB2)
return vcat(gens(I), GB1, GB2)
Copy link
Collaborator

Choose a reason for hiding this comment

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

You are performimg two additional GB computations here. I agree that adding smaller elements may be helpful with respect to speed but is this also true when you use GB computations to find the elements?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I made some "randomly generated" tests, and it was generally faster (and only rarely a bit slower) when using these GB computations. If you have some "real" inputs, we can test them. In summary: the costs of these two GB computations were (usually) lower than the speed gain when computing the saturation. Conceivably, this may change when Hans has a better implementation of saturation -- the question should then be revisited.
Of course, if you have a better way of generating further "small" elements of the ideal, let me know.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Of course, your subsequent test is_one(I) mentioned already above computes a REDUCED degrevlex GB which would be completely superfluous whenever the ideal is not the whole ring. In total you introduced 3 superfluous GB computations. The subsequent saturation step would detect whether the original ideal is the whole ring anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am considering changing the "short-cut" case to

 if all(is_homogeneous, gens(I)) 
        Hgens = homogenization(gens(I))
        return ideal(parent(Hgens), Hgens)

The zero ideal needs some special handling (which I have omitted here).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah! The weight matrix W is an input parameter, and not intrinsic to the graded ring... ?1?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I do not understand. What is the short-cut case? Also, I do not understand your code. You homogenize the generators with respect to the standard ZZ-grading if they are homogeneous with respect to the standard ZZ-grading?

And which weight matrix do you mean?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, maybe the "short-cut case" is useless: I meant if the given generators already happen to be homogeneous (with respect to W). I'll ask Hans whether saturation by a variable which does not appear in the generators as handled efficiently... if so, then my "short-cut" is quite unnecessary. The call to homogenization when the generators are already homogeneous is simply to map them into the correct ring [as you pointed out].
I am puzzled as to why the input parameters to homogenization(I,W,var,pos) include W, var and pos. I would have expected these 3 parameters to be "instrinsic" to the graded polynomial ring. The documentation says that is_homogeneous takes just 1 argument; and the examples suggest that the grading used is that of the graded polynomial ring. So it is not clear to me why I must specify W when calling homogenization. To some extent I can understand why var and pos are necessary (though it seems like they should be recorded inside the graded polynomial ring along with the weight matrix W)

Copy link
Collaborator

Choose a reason for hiding this comment

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

The point here is that, typically, we do not have the graded ring yet. That is, I is not an ideal of a graded ring . So you need to tell the function with respect to which grading it should homogenize. One method for is_homogeneous is Bill's AA version of is_homogeneous which checks whether a polynomial is homogeneous with respect to the standard ZZ-grading (so this applies also to polynomials in non-graded rings, but has a specific grading in mind). The methods is_homogeneous we implemented in addition apply to elements of graded rings only. So this is another case of confusion arising from the history of AA.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah! So in practice I cannot use is_homogeneous on the input generators, right? That excludes the "short-cut".
Indeed it is confusing that "homogenization" has such a context-dependent meaning. I must review my code.
I also spoke to Hans: Singular does not yet handle specially the case of saturating by variables which do not appear in the generators.

function num_terms(f)
return length(collect(coefficients(f)));
end;

Copy link
Contributor

Choose a reason for hiding this comment

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

just use length(f) this avoids the extremely expensive collect(coefficients
Many iterators also support length, so even length(coefficients(f)) would be better

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. I've updated my local copy.

@wdecker
Copy link
Collaborator

wdecker commented May 17, 2023

@JohnAAbbott The problems I have with this PR are not yet resolved (see comments above). I suggest to make this a draft until we have met in person again.

@JohnAAbbott
Copy link
Contributor Author

@wdecker
I have removed the code which "wanted to check" whether the given generators are already homogeneous because it did not work as hoped: is_homogeneous defaults to standard grading. Anyway, it is probably best to delegate to Singular the task of spotting simplifications when computing the saturation.
The "superfluous" GB computations in gens_for_homog are mathematically unnecessary, but in my tests the complete homogenization computation (incl. the cost of gens_for_homog) was often significantly faster than when using just the supplied generators; there were some cases where the complete homogenization computation became a bit slower (but I have not seen any cases where it became significantly slower). Of course, I can replace

    Hgens = homogenization(gens_for_homog(I), W, var, pos)

with the simpler line

    Hgens = homogenization(gens(I), W, var, pos)

but as I have just said that will typically make homogenization slower (sometimes significantly slower).

@JohnAAbbott
Copy link
Contributor Author

@wdecker
Hans suggested a compromise: inside gens_for_homog check whether a GB is already known, and if so, use any that are there. This should guarantee that gens_for_homog is quick, but produces a benefit only if some GB has already been cached. My guess is that a DegRevLex GB computation for the original ideal is likely considerably cheaper than the GB computation inside the computation of the saturation.

@wdecker
Copy link
Collaborator

wdecker commented May 17, 2023 via email

@fingolfin
Copy link
Member

So Hans fixed this in Singular and will make a new Singular_jll release soon. And then hopefully we can also soon have a new Singular.jl release (this is partially waiting on me).

@JohnAAbbott
Copy link
Contributor Author

Saturation by a principal ideal generated by a product is equivalent to a cascade of saturations by the factors: see Kreuzer+Robbiano Book 1, Tutorial 37 part (g).
Saturation by a single indeterminate: see Kreuzer+Robbiano book 2 Corollary 4.4.9.

@JohnAAbbott
Copy link
Contributor Author

Unless I am mistaken there should now be the completely new implementation accessible via the new function homogenization2 which is intended to be a drop-in replacement for homogenization (just for ideals).
By using a temporary different name once can test and compare the two versions. I know of no case where the old code is as fast as the new code (i.e. homogenization2 should always be faster, usually much faster).
The newly added code deliberately does not follow OSCAR coding standards -- I plan to "clean" it when the PR is no longer a draft.

@JohnAAbbott JohnAAbbott marked this pull request as draft June 30, 2023 15:05
@JohnAAbbott JohnAAbbott marked this pull request as ready for review July 24, 2023 14:50
@JohnAAbbott JohnAAbbott reopened this Jul 27, 2023
@wdecker wdecker merged commit 1deabec into oscar-system:master Jul 27, 2023
22 of 29 checks passed
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

4 participants