Minimal associated primes #3705
Conversation
|
@ederc : Could you have a look at the tests? I don't understand what's going wrong. But maybe |
|
Thanks for wrapping this @HechtiDerLachs ! |
|
Well, the tests fail because you try to apply |
|
The problem I meant occured in an earlier test run. Some call to But what you say about |
|
There was a missing caching of the |
|
is there anything holding this PR up @HechtiDerLachs ? |
|
The tests were still failing. I just had a look and it seems there is another bug in |
HechtiDerLachs
force-pushed
the
minimal_associated_primes
branch
from
9aa5177
to
2cd696e
Compare
|
@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! |
|
I do not have this code anymore, we need to look again where the |
The Github UI shows |
|
@HechtiDerLachs Caching |
|
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 ReportAll modified and coverable lines are covered by tests ✅
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
|
|
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. |
|
Its still under Hans aegis:
|
|
@hannes14 updated Singular_jll and just told me that Singular.jl should automatically use that. So I've now restarted the tests, let's see if it helps. |
|
I looked into the errors / warnings a bit, so far I have found the following: This is triggered from an atexit handler from the perl code which tries to clean up polymake objects. Not totally sure why this error doesn't appear during normal process exit, maybe because some other finalizers need to run first but these are not triggered correctly for these processes. This is also during process exit, maybe because multiple processes are trying to modify the same files at the same time (and this is not really a place where we can add a pidlock). All these processes are trying to save the same new auto-generated code that was probably triggered in the original process. This is in a macos job and I don't really know what exactly triggers this, but this could also be due to code running at exit without some julia exit handlers running before this, maybe related to this: Note: Many of the CI jobs also timed out, something got a lot slower here or got stuck somewhere? |
|
I ran the Oscar tests on this branch locally and after a while it also got stuck with many processes (22 processes including the main one), all of them seem to be sleeping / waiting. The main process (28895) is deep inside some singular code, called from One Process (1803) is in a wait call: One Process (1843) is in a futex_wait (called from some openmp exit handlers?): And finally there are further 19 processes which are waiting in a read like this: I can keep those processes alive for a while if there is anything I should check, they don't need much memory and all are idle anyway. |
|
The |
|
There are still exit handlers running 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 |
|
@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? |
|
Singular uses sysconf(_SC_NPROCESSORS_ONLN) resp. sysconf(_SC_NPROCESSORS_CONF), |
src/Rings/mpoly-ideals.jl
Outdated
| 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]...)) |
There was a problem hiding this comment.
| 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])) |
| 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 |
There was a problem hiding this comment.
@HechtiDerLachs I resolved the merge conflicts -- I hope I didn't screw that up too badly...
|
Tests pass now, but |
| unique_comp = typeof(I)[] | ||
| for q in J | ||
| is_one(q) && continue | ||
| q in unique_comp && continue |
There was a problem hiding this comment.
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?)
There was a problem hiding this comment.
@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.
src/Rings/mpoly-ideals.jl
Outdated
| @@ -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)) | |||
There was a problem hiding this comment.
dim(I) triggers a GB computation?
There was a problem hiding this comment.
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.
|
I can confirm that the test timings go up. The reason was not the call to Here is an example. Therefore it should not be the default algorithm. Maybe there exists input where it is faster. I do not know. |
There was a problem hiding this comment.
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.
src/Rings/mpoly-ideals.jl
Outdated
| Calls a specialized variant of `minimal_primes` for zero dimensional ideals | ||
| with coefficient ring over the rationals. |
There was a problem hiding this comment.
| 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. |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
My 2 pennies:
|
|
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: ***@***.***>
|
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
QQwas allowed as a coefficient ring. Do you remember whether this is the case? Because it seems to have run also over number fields.