Intermediate PR for work on elliptic fibrations #3564
Conversation
| morphism_of_projective_schemes(P::AbsProjectiveScheme, Q::AbsProjectiveScheme, f::Map; check::Bool=true ) | ||
| morphism_of_projective_schemes(P::AbsProjectiveScheme, Q::AbsProjectiveScheme, f::Map, h::SchemeMor; check::Bool=true ) | ||
| morphism_of_projective_schemes(X::AbsProjectiveScheme, Y::AbsProjectiveScheme, a::Vector{<:RingElem}) | ||
| morphism(P::AbsProjectiveScheme, Q::AbsProjectiveScheme, f::Map; check::Bool=true ) | ||
| morphism(P::AbsProjectiveScheme, Q::AbsProjectiveScheme, f::Map, h::SchemeMor; check::Bool=true ) | ||
| morphism(X::AbsProjectiveScheme, Y::AbsProjectiveScheme, a::Vector{<:RingElem}) |
There was a problem hiding this comment.
@simonbrandhorst : Do you know when this happened? What is the anticipated format? Didn't we decide for plain morphism at some point?
There was a problem hiding this comment.
no clue, but we did decide for morphism
There was a problem hiding this comment.
Needs a deprecation though. The function is exported and in src.
| d = F2 - _vec(tor) | ||
| d = F2-vec(tor) |
There was a problem hiding this comment.
@simonbrandhorst : Do you remember what was going on here in this file?
There was a problem hiding this comment.
the changes to linear algebra required changes elsewhere as well
There was a problem hiding this comment.
vec for Nemo matrices was renamed _vec
| #= | ||
| function (f::Oscar.MPolyAnyMap{<:MPolyRing, <:MPolyQuoLocRing, <:MPolyQuoLocalizedRingHom})(a::MPolyRingElem) | ||
| if !has_attribute(f, :lifted_map) | ||
| g = get_atttribute!(f, :lifted_map) do | ||
| S = domain(f) | ||
| W = codomain(f) | ||
| L = localized_ring(W) | ||
| g = hom(S, L, x -> lift(f.coeff_map(x)), lift.(f.img_gens), check=false) | ||
| set_attribute!(f, :lifted_map, g) | ||
| end | ||
| g = get_attribute(f, :lifted_map) | ||
| return codomain(f)(g(a), check=false) | ||
| end::Map{typeof(domain(f)), typeof(localized_ring(codomain(f)))} | ||
|
|
||
| b = g(a)::MPolyLocRingElem | ||
| return codomain(f)(numerator(b), denominator(b), check=false) | ||
| end | ||
| =# |
There was a problem hiding this comment.
This seemed a bit awkward. If commenting it out does not cause the tests to fail, then we should remove it.
I have looked through the last few CI logs but cannot find them. If they occur again, can you paste a link here? |
I assume he means this: https://github.com/oscar-system/Oscar.jl/actions/runs/8601641041/job/23569386555. |
|
Another Aqua failure: https://github.com/oscar-system/Oscar.jl/actions/runs/8602290333/job/23571530648?pr=3564 |
| @test length(small_gens) == 3 # saturated_ideal is not called here. | ||
| #@test isone(first(small_gens)) |
There was a problem hiding this comment.
We should have both variants available. Depending on the context both make sense. What about calling your variant _small_generating_set_nosatideal?
There was a problem hiding this comment.
If one wants the small generating set of the saturated ideal, one can ask for that. I see no need to introduce an extra function really. Relying on the saturated ideal in general has proved to be a severe bottleneck.
There was a problem hiding this comment.
For me this is the distinction between interactive user (small_generating_set) and targeting performance (where saturated_ideal should be avoided whereever possible).
Co-authored-by: Johannes Schmitt <[email protected]>
| """ | ||
| function simplify(I::IdealSheaf, cov::Covering=default_covering(scheme(I))) | ||
| id_dict = IdDict{AbsAffineScheme, Ideal}( | ||
| U => ideal(OO(U), small_generating_set(saturated_ideal(I(U)))) for U in patches(cov) |
There was a problem hiding this comment.
Why is there a saturated ideal here?
Shouldn't this be taken care off by the small_generating_set function instead?
There was a problem hiding this comment.
Currently small_generating_set does not have a method for this signature. But we could introduce one for localizations at D(f), which uses saturated_ideal internally.
There was a problem hiding this comment.
small_generating_set does not compute a saturation anymore. It's not what the name of the function suggests and it killed performance in some cases. So I threw it away.
|
I will make a new PR for this which is rebased on the latest master. |
This is work in progress with @simonbrandhorst .
We would like to try to get some changes in already.
Edit: This has proved to be more nasty than expected. I will get back to it next week.