Minimal associated primes #3705

HechtiDerLachs · 2024-05-08T12:12:04Z

An attempt to make the specialized functionality in Singular for zero dimensional ideals available.

This seemed to be useful for @simonbrandhorst in some examples, but now I can't even get the tests to terminate. Let's see what the CI says.

@wdecker : The documentation reads as if only QQ was allowed as a coefficient ring. Do you remember whether this is the case? Because it seems to have run also over number fields.

HechtiDerLachs · 2024-05-08T12:54:02Z

@ederc : Could you have a look at the tests? I don't understand what's going wrong. But maybe minimal_primes now called singular_generators before and that lead to some false caching?

simonbrandhorst · 2024-05-08T17:08:30Z

Thanks for wrapping this @HechtiDerLachs !

ederc · 2024-05-08T17:56:59Z

Well, the tests fail because you try to apply assPrimes from Singular on a zero dimensional ideal over a finite field, but assPrimes assumes that the ideal lives over QQ. AFAIK there is a modular std call behind the scenes in assPrimes.

HechtiDerLachs · 2024-05-08T18:06:08Z

The problem I meant occured in an earlier test run. Some call to dim was complaining that singular_groebner_generators did not return a groebner basis. But it doesn't seem to be reproduced anymore; sorry for calling you.

But what you say about minAss is good to know. I will restrict the cases where it's called.

HechtiDerLachs · 2024-05-09T08:34:28Z

@ederc : Now the error is back, see this test.

ederc · 2024-05-09T19:24:33Z

There was a missing caching of the isGB flag if a GB was computed during small_generating_set. I pushed a fix to your branch.

simonbrandhorst · 2024-06-26T09:00:48Z

is there anything holding this PR up @HechtiDerLachs ?

HechtiDerLachs · 2024-06-26T15:26:23Z

The tests were still failing. I just had a look and it seems there is another bug in minimal_primes (due to me, unfortunately). I will have a look.

HechtiDerLachs · 2024-06-29T19:33:11Z

@ederc : I'm sorry, but it looks like I accidentally overwrote your fixes to this branch when doing a rebase. Do you still have them somewhere? If yes, could you push them here again? Thx!

ederc · 2024-06-29T20:00:19Z

I do not have this code anymore, we need to look again where the isGB flag needs to be set.

benlorenz · 2024-06-30T10:22:04Z

@ederc : I'm sorry, but it looks like I accidentally overwrote your fixes to this branch when doing a rebase. Do you still have them somewhere? If yes, could you push them here again? Thx!

The Github UI shows HechtiDerLachs force-pushed the minimal_associated_primes branch from 9aa5177 to 2cd696e, and clicking on that 9aa5177 commit should get you to the last commit before the force push, and the history is also available. With that commit hash known you can also try doing git log 9aa5177 locally since git will keep lost commits for a few days.
You could also look through git reflog locally.

ederc · 2024-06-30T16:05:51Z

@HechtiDerLachs Caching isGB is back in.

HechtiDerLachs · 2024-06-30T18:10:09Z

Thanks a lot @ederc and @benlorenz ! I was hoping that something like this was possible, but didn't know how.

Unfortunately it seems that a lot of tests time out. Or something else goes wrong which I do not fully understand, yet.

Edit: I checked two of the failing tests locally around the point where they were cancelled and they run just fine on my machine.

codecov · 2024-07-01T06:20:58Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 84.48%. Comparing base (020e83a) to head (1d68e74).
Report is 19 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #3705      +/-   ##
==========================================
- Coverage   84.59%   84.48%   -0.12%     
==========================================
  Files         631      641      +10     
  Lines       85085    85440     +355     
==========================================
+ Hits        71981    72181     +200     
- Misses      13104    13259     +155

Files with missing lines	Coverage Δ
src/Rings/mpoly-ideals.jl	`94.19% <100.00%> (+0.31%)`	⬆️

... and 40 files with indirect coverage changes

HechtiDerLachs · 2024-07-01T08:56:31Z

I really can't make sense of the failing tests. Everything that I tried on my local machine about things where the CI gets stuck really goes through for me. And in some cases I can't even find the error messages.

fingolfin · 2024-07-18T14:11:03Z

Triage talked about it yesterday and we suspect that it's the same problem as in #3905 i.e. Singular tries to start subprocesses and gets stuck. So hopefully @hannes14 can take a look when he's back from vacation.

fieker · 2024-08-28T09:38:24Z

Its still under Hans aegis:

it is fixed (in Singular)
it needs the release cascade
(maybe)

fingolfin · 2024-09-16T22:15:23Z

The atexit related crashes are fixed in @hannes14 most recent update from last week. But there are still errors / crashes, just again different ones

benlorenz · 2024-09-17T06:49:31Z

There are still exit handlers running

│    Can't call method "flags" on an undefined value at /Users/aaruni/Desktop/oscar-runners/runner-2/julia-depot/scratchspaces/d720cf60-89b5-51f5-aff5-213f193123e7/polymake_6360604600323823479_1.10_depstree_v2/share/polymake/perllib/Polymake/Core/BigObject.pm line 1247 during global destruction.

which I think is problematic. But at least I don't see any stuck jobs that ran into a timeout.

Many of the failing jobs don't have any logs for the Run tests step, some show an exit code of 143 (sigterm) which might indicate the workers running out of memory? (due to multiple processes?)

fingolfin · 2024-09-20T09:30:29Z

@hannes14 so I guess that ball is in your court again ;-).

Out of curiosity, how does Singular determine how many processes to fork? Does it take the number of cores and/or RAM into account?

hannes14 · 2024-09-20T09:53:36Z

Singular uses sysconf(_SC_NPROCESSORS_ONLN) resp. sysconf(_SC_NPROCESSORS_CONF),
RAM is not considered

lgoettgens · 2024-09-21T08:36:33Z

src/Rings/mpoly-ideals.jl

+    result = typeof(I)[]
+    # `unique!` does not work for lists of ideals. I don't know why, but for the moment we need the 
+    # following workaround.
+    for p in filter!(!is_one, vcat([minimal_primes(j; algorithm, factor_generators=false) for j in J]...))


Suggested change

for p in filter!(!is_one, vcat([minimal_primes(j; algorithm, factor_generators=false) for j in J]...))

for p in filter!(!is_one, reduce(vcat, [minimal_primes(j; algorithm, factor_generators=false) for j in J]))

fingolfin · 2024-10-09T00:08:17Z

src/Rings/mpoly-ideals.jl

+    unique_comp = typeof(I)[]
+    for q in J
+      is_one(q) && continue
+      q in unique_comp && continue
+      push!(unique_comp, q)
+    end
+    J = unique_comp


@HechtiDerLachs I resolved the merge conflicts -- I hope I didn't screw that up too badly...

fingolfin · 2024-10-09T09:39:24Z

Tests pass now, but test/AlgebraicGeometry/Schemes/Resolution_structure.jl in Julia 1.10 went from ~64 seconds to ~370 seconds

fingolfin · 2024-10-30T10:52:35Z

src/Rings/mpoly-ideals.jl

+    unique_comp = typeof(I)[]
+    for q in J
+      is_one(q) && continue
+      q in unique_comp && continue


This compares ideals, won't that trigger additional GB computations and thus be slow in general?? Maybe you just want to remove "obviously" identical ideals (with generators lists being the same, up to reordering?)

@fingolfin : is_one(q) triggers a GB computation, but (at least hopefully) the ideal membership test in 1240 will use those GB from the prior runs of the loop for the comparison.

I rather wonder whether the factorization in Oscar is so much superior that it does not make sense to do a facstd of J in Singular and then go on with the components from there.

Edit: If it is about working over field extensions, I understand the point.

fingolfin · 2024-10-30T10:53:18Z

src/Rings/mpoly-ideals.jl

@@ -1126,6 +1126,12 @@ julia> L = minimal_primes(I)
 function minimal_primes(I::MPolyIdeal; algorithm::Symbol = :GTZ, cache::Bool=true)
  has_attribute(I, :minimal_primes) && return get_attribute(I, :minimal_primes)::Vector{typeof(I)}
  R = base_ring(I)
+  if coefficient_ring(R) isa QQField && is_zero(dim(I))


dim(I) triggers a GB computation?

Triage suggest this would perhaps make sense with is_known_to_be_zero_dimensional(I) check -- i.e. if we know this information, we can call the 0-dim method.

But otherwise, leave it singular -- if it detects the 0-dim case it still handles it well.

simonbrandhorst · 2024-11-01T13:45:07Z

I can confirm that the test timings go up. The reason was not the call to dim but that
Singular.LibAssprimeszerodim.assPrimes was slower in all cases I could produce.

Here is an example.

julia> R, (x, y) = polynomial_ring(QQ, [:x, :y])
(Multivariate polynomial ring in 2 variables over QQ, QQMPolyRingElem[x, y])

julia> I = ideal(R,[x^5*y - 2*x^4*y^2 - 2*x^4*y + x^3*y^3 + 2*x^3*y^2 - x^2*y^3 + 2*x^2*y^2 + 2*x^2*y + 2*x*y^4 + x*y^3 - 2*x*y^2 - x*y - y^5 - 2*y^4 - y^3, x^6 - 2*x^5 - 3*x^4*y^2 - 2*x^4*y + 2*x^3*y^3 + 3*x^3*y^2 + 2*x^3*y + 2*x^3 + 5*x^2*y^2 + 2*x^2*y - x^2 + 3*x*y^4 - 5*x*y^2 - 2*x*y - 2*y^5 - 4*y^4 - 2*y^3, x^7 + y^3 - 6]
       )
Ideal generated by
  x^5*y - 2*x^4*y^2 - 2*x^4*y + x^3*y^3 + 2*x^3*y^2 - x^2*y^3 + 2*x^2*y^2 + 2*x^
  2*y + 2*x*y^4 + x*y^3 - 2*x*y^2 - x*y - y^5 - 2*y^4 - y^3
  x^6 - 2*x^5 - 3*x^4*y^2 - 2*x^4*y + 2*x^3*y^3 + 3*x^3*y^2 + 2*x^3*y + 2*x^3 + 
  5*x^2*y^2 + 2*x^2*y - x^2 + 3*x*y^4 - 5*x*y^2 - 2*x*y - 2*y^5 - 4*y^4 - 2*y^3
  x^7 + y^3 - 6

julia> @time minimal_primes(I)
  0.031095 seconds (54.79 k allocations: 1.706 MiB)
2-element Vector{MPolyIdeal{QQMPolyRingElem}}:
 Ideal with 2 generators
 Ideal (y^7 + 7*y^6 + 21*y^5 + 35*y^4 + 36*y^3 + 21*y^2 + 7*y - 5, x - y - 1)

julia> @time Oscar.minimal_primes_zero_dim(I)
 20.402501 seconds (108.43 k allocations: 1.781 MiB)
2-element Vector{MPolyIdeal{QQMPolyRingElem}}:
 Ideal with 5 generators
 Ideal with 5 generators

julia> dim(I)
0

Therefore it should not be the default algorithm. Maybe there exists input where it is faster. I do not know.
do you ? @hannes14
Since this PR also contains some fixes, I suggest to move forward with it.

afkafkafk13

The two bug fixes make sense.

The benefit of the new method is rather limited and seems to rely on Singular's parallization via modular.lib. Hence we should at least inform the user that this is using a modular approach. -- minor change, but approval has to wait for it.

afkafkafk13 · 2024-11-04T17:17:33Z

src/Rings/mpoly-ideals.jl

+Calls a specialized variant of `minimal_primes` for zero dimensional ideals 
+with coefficient ring over the rationals.


Suggested change

Calls a specialized variant of `minimal_primes` for zero dimensional ideals

with coefficient ring over the rationals.

Calls a specialized modular variant of `minimal_primes` for zero dimensional ideals

with coefficient ring over the rationals.

What should one take away from this additional information? That the result may not be correct anymore? Consider me a user/developer, who stumbles upon this method and reads the docstring.

Any better hint for the knowledgable user that there is a modular approach and Singular-level parallelization going on in the background is welcome -- but it needs to be of a kind that does not disturb the average user.

(I might not even have included this function into the list of exported commands -- i.e. put a _ in front of it --, if it had been my choice.... It is very often significantly inferior to the usual method from primdec.lib concerning time consumption due to the overhead.)

I can already not understand why I should write specialized here. This alone gives no information. Specialized modular is even more confusing. Two comments:

This Singular function comes with final tests (depending on the value). I will check asap what precisely these checks are doing and whether they guarantee correctness (correctness in some cases).

As we will more and more rely on modular GB based methods, we should develop a general philosophy of telling the truth to the user. In contrast to Magma, we should not cheat here.

With respect to this PR, let us decide on how to handle this as soon as I have checked what the tests do.

Indeed the word specialized should be deleted, whereas I would keep modular.

I strongly second the need for a general philosophy as stated in 2. .

W.r.t. this PR: What we definitely need are the two bug fixes.

Thanks for the feedback. I removed specialized and added modular.

So far I did not export the function because I could not determine a clear use-case for it.

src/Rings/mpoly-ideals.jl

fieker · 2024-11-05T07:08:18Z

My 2 pennies:

adding modular is pointless as no-one outside knows what this implies. Maybe call it Las Vegas/ Monte Carlo/ .. whatever to indicate that is is not guarateed correct.
any function in Singular that is using Singular parallelization, at this point in time, cannot be used in Oscar reliably. Thus I'd question the point of linking them in. It needs structural changes in Singular before they can be used at all. This is under discussion. Even the feasibility is not clear yet. Unfortunately.

wdecker · 2024-11-05T07:32:00Z

Again, we need to be more precise here. Some of this is guaranteed to be correct provided the neccessary checks are on. For the case considered here, I will find out soon.Von meinem iPhone gesendetAm 05.11.2024 um 08:08 schrieb Claus Fieker ***@***.***>: My 2 pennies: adding modular is pointless as no-one outside knows what this implies. Maybe call it Las Vegas/ Monte Carlo/ .. whatever to indicate that is is not guarateed correct. any function in Singular that is using Singular parallelization, at this point in time, cannot be used in Oscar reliably. Thus I'd question the point of linking them in. It needs structural changes in Singular before they can be used at all. This is under discussion. Even the feasibility is not clear yet. Unfortunately. —Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you were mentioned.Message ID: ***@***.***>

jankoboehm · 2024-11-06T09:56:05Z

My two pennies: We have to separate three different issues here:

modular or not:
modular is just an algorithmic option to the command and there should always be a non-modular version if possible (since modular in general is not for all inputs faster).

verification:
If modular, one should be able to turn verfication on/off via an algorithmic option to the command or via a global "physics/science" mode, or both. As standard, verification has to be on.

resources:
Even on one core modular can be for some algorithms much faster that non-modular. We should enhance Singular (i.e. tasks.lib, resources.lib, parallel.lib) with a special case so that it does not fork if we use a single core. This does not harm even if the forking works smoothly eventually.

afkafkafk13 · 2024-11-06T10:18:42Z

My two pennies: We have to separate three different issues here:

modular or not: modular is just an algorithmic option to the command and there should always be a non-modular version if possible (since modular in general is not for all inputs faster).

This was, what I had in mind: Be frank about this being a modular method (as opposed to the non-modular approach in primdec.lib).

verification: If modular, one should be able to turn verfication on/off via an algorithmic option to the command or via a global "physics/science" mode, or both. As standard, verification has to be on.

Here the tests are switched on by default, so this is indeed verified (and certifies correctness according to the underlying article of Idrees, Pfister, Steidel). But the Oscar-method in this PR currently does not provide the means to turn it off yet. Is it desired to be able to switch it off?

resources: Even on one core modular can be for some algorithms much faster that non-modular. We should enhance Singular (i.e. tasks.lib, resources.lib, parallel.lib) with a special case so that it does not fork if we use a single core. This does not harm even if the forking works smoothly eventually.

This is a valid point in general, but not of relevance here. The current PR is an instance where no parallelism is used at all in Singular and computation only passes through the modular command.
Edit: I take the last phrase back.

afkafkafk13 · 2024-11-06T12:04:23Z

Please revert to draft for the moment. This touches on things that need to be checked/cleaned up in Singular before having this in Oscar.

simonbrandhorst · 2024-11-06T12:24:15Z

This should not delay the two bugfixes. I will remove the singular part from this PR and return to non-draft.

afkafkafk13 · 2024-11-06T12:29:33Z

Splitting up this PR into the urgent part to be merged asap and the one touching on Singular is fine with me.

HechtiDerLachs marked this pull request as draft May 8, 2024 12:12

HechtiDerLachs mentioned this pull request May 8, 2024

Difficulties with radical computation #3711

Open

HechtiDerLachs added 3 commits June 29, 2024 21:17

Hook in zero dimensional primary decomposition.

8ffdfb1

Add tests.

6f1fcc1

Restrict usages of special decomposition.

2cd696e

HechtiDerLachs force-pushed the minimal_associated_primes branch from 9aa5177 to 2cd696e Compare June 29, 2024 19:18

Fix initialization.

c6e89fe

Avoid usage of unique.

af7a9c4

adds caching of isGB information

70905de

HechtiDerLachs closed this Jul 1, 2024

HechtiDerLachs reopened this Jul 1, 2024

HechtiDerLachs marked this pull request as ready for review July 1, 2024 09:01

HechtiDerLachs added the triage label Jul 1, 2024

lgoettgens reviewed Sep 21, 2024

View reviewed changes

Merge branch 'master' into minimal_associated_primes

aa41ef1

fingolfin reviewed Oct 9, 2024

View reviewed changes

fingolfin reviewed Oct 30, 2024

View reviewed changes

simonbrandhorst added 3 commits November 1, 2024 13:30

Merge branch 'master' into minimal_associated_primes

2cab9ef

introduce minimal_primes_zero_dim

4cecea8

remove is_known_to_be_zero_dimensional again

43956ce

fix typo add test

d299941

simonbrandhorst requested review from wdecker and afkafkafk13 November 4, 2024 12:14

afkafkafk13 reviewed Nov 4, 2024

View reviewed changes

simonbrandhorst reviewed Nov 4, 2024

View reviewed changes

src/Rings/mpoly-ideals.jl Outdated Show resolved Hide resolved

Update src/Rings/mpoly-ideals.jl

1d68e74

lgoettgens marked this pull request as draft November 6, 2024 12:09

simonbrandhorst mentioned this pull request Nov 6, 2024

Fixes small_generating_set, mininimal_primes over a number field #4279

Merged

Merge branch 'master' into minimal_associated_primes

0f5e6cd

	for p in filter!(!is_one, vcat([minimal_primes(j; algorithm, factor_generators=false) for j in J]...))
	for p in filter!(!is_one, reduce(vcat, [minimal_primes(j; algorithm, factor_generators=false) for j in J]))

		Calls a specialized variant of `minimal_primes` for zero dimensional ideals
		with coefficient ring over the rationals.

Minimal associated primes #3705

Are you sure you want to change the base?

Minimal associated primes #3705

Conversation

HechtiDerLachs commented May 8, 2024

HechtiDerLachs commented May 8, 2024

simonbrandhorst commented May 8, 2024

ederc commented May 8, 2024

HechtiDerLachs commented May 8, 2024

HechtiDerLachs commented May 9, 2024

ederc commented May 9, 2024

simonbrandhorst commented Jun 26, 2024

HechtiDerLachs commented Jun 26, 2024

HechtiDerLachs commented Jun 29, 2024

ederc commented Jun 29, 2024

benlorenz commented Jun 30, 2024

ederc commented Jun 30, 2024

HechtiDerLachs commented Jun 30, 2024 • edited Loading

codecov bot commented Jul 1, 2024 • edited Loading

Codecov Report

HechtiDerLachs commented Jul 1, 2024

fingolfin commented Jul 18, 2024

fieker commented Aug 28, 2024

fingolfin commented Sep 16, 2024

benlorenz commented Sep 17, 2024

fingolfin commented Sep 20, 2024

hannes14 commented Sep 20, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fingolfin commented Oct 9, 2024

Choose a reason for hiding this comment

afkafkafk13 Nov 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simonbrandhorst commented Nov 1, 2024

afkafkafk13 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

afkafkafk13 Nov 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fieker commented Nov 5, 2024

wdecker commented Nov 5, 2024 via email

jankoboehm commented Nov 6, 2024 • edited Loading

afkafkafk13 commented Nov 6, 2024 • edited Loading

afkafkafk13 commented Nov 6, 2024

simonbrandhorst commented Nov 6, 2024

afkafkafk13 commented Nov 6, 2024

HechtiDerLachs commented Jun 30, 2024 •

edited

Loading

codecov bot commented Jul 1, 2024 •

edited

Loading

afkafkafk13 Nov 4, 2024 •

edited

Loading

afkafkafk13 Nov 4, 2024 •

edited

Loading

jankoboehm commented Nov 6, 2024 •

edited

Loading

afkafkafk13 commented Nov 6, 2024 •

edited

Loading