-
Notifications
You must be signed in to change notification settings - Fork 123
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
Some more fixes #4142
Some more fixes #4142
Conversation
experimental/DoubleAndHyperComplexes/src/Morphisms/simplified_complexes.jl
Outdated
Show resolved
Hide resolved
Yes, I'm already working on it. It's a bit annoying, because I had to work on the compute-server today, but there I can't put my login-credentials for GH. Thus, I manually copied the files via scp and this is what you get... |
f27b4d0
to
93c4575
Compare
@@ -1149,7 +1170,14 @@ function minimal_primes( | |||
end | |||
J = K | |||
end | |||
result = unique!(filter!(!is_one, vcat([minimal_primes(j; algorithm, factor_generators=false) for j in J]...))) | |||
# unique! seems to fail here. We have to do it manually. | |||
pre_result = filter!(!is_one, vcat([minimal_primes(j; algorithm, factor_generators=false) for j in J]...)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pre_result = filter!(!is_one, vcat([minimal_primes(j; algorithm, factor_generators=false) for j in J]...)) | |
pre_result = filter!(!is_one, reduce(vcat, [minimal_primes(j; algorithm, factor_generators=false) for j in J])) |
save some allocations
src/Rings/mpoly-ideals.jl
Outdated
# Since most ideals implement `==`, they have to implement the hash function. | ||
# See issue #4143 for problems entailed. Interestingly, this does not yet fix | ||
# the failure of unique! on lists of ideals. | ||
function hash(I::Ideal, c::UInt) | ||
return hash(typeof(I), hash(base_ring(I), c)) | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I advocate for this solution now. It is a matter of fact that ideals can not be hashed in general and even for polynomial rings this is probably expensive if not impossible (we need to choose an ordering which might be a dead end for a specific ideal). Yet, the ==
function is vital to how we use ideals and we need to be able to use them in vectors/lists/dictionaries.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it safe to hash the type? After all two mathematically equal objects may have different types.
(Thinking e.g. of IdealSheaf, RadicalIdealSheaf, PrimeIdealSheaf etc.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it safe to hash the type? After all two mathematically equal objects may have different types. (Thinking e.g. of IdealSheaf, RadicalIdealSheaf, PrimeIdealSheaf etc.)
It is only safe if you don't implement ==
between this type and other types. In your example, you shouldn't include the type in hashes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, good to know. I was only following the suggestion by @benlorenz . But you're probably right.
Co-authored by afkafkafk13
I am sorry about the messy state that this is in. But it turned out to be a really hard nut to crack here. Some things to be discussed:
Once this reaches a state where tests actually pass, I will remove the code which for the time being has just been commented out. But if you have any objections about where parts of this are going, please let me know already. |
Can you clarify what you mean by 'simultaneous blowup strategy'? |
In the elliptic surfaces we exclusively encounter A-D-E singularities. Up to now we blew them up one by one. But this produces a lot of charts and gluings. In the particular example we need for our paper we only have 8 ordinary A1 singularities. They are resolved simultaneously by one single blowup. This is achieved with with a keyword argument to the constructor for elliptic surfaces which then selects this strategy: Just blow up the ideal sheaf of the reduced singular locus, until you're done. |
oh. I see. This makes a lot of sense. |
ref_cod, a, b = _register!(common_refinement(codomain(f_cov), codomain(inc_cod_cov)), codomain(f)) | ||
inc_cod_ref = restrict(inc_cod, ref_cod) | ||
f_res = restrict(f, ref_cod) | ||
ref_dom, aa, bb = _register!(common_refinement(domain(f_res), codomain(inc_dom_cov)), domain(f)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Before the refinements were registered. Now this does not seem to be the case anymore.
Is this intentional? And if yes what is the rationale here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Originally we were planning to keep track of all the refinements and the morphisms between them inside the covered scheme. But this is not very garbage-collector-friendly, so we eventually refrained from that and switched to the implicit tree structure for refinements. The call to _register!
was a deprecated artefact.
I am fine with discontinuing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changes in the doctests in unrelated code such as algebraic statistics seem odd.
Can you explain the change in behavior? Are the results still correct?
See #4171 and my comment on slack. |
O.K. when you checkout the files I reapprove. |
Today's harvest from working on the elliptic surfaces.