Improve coverings, round II #1855
Conversation
| V = ambient_scheme(U) | ||
| IV = F(V)::Ideal | ||
| rho = OOX(V, U) | ||
| IU = ideal(OO(U), rho.(gens(IV))) | ||
| return IU | ||
| end | ||
| function production_func(F::AbsPreSheaf, U::SimplifiedSpec) | ||
| haskey(ID, U) && return ID[U] | ||
| V = original(U) |
There was a problem hiding this comment.
Original goes all the way up to the original covering of X, right? Then is is fine.
There was a problem hiding this comment.
No. It only goes up one step.
There was a problem hiding this comment.
So we call F(V) one step upstairs, which then calls production_func with the appropriate input and we recurse our way up?
| UU, _ = glueing_domains(default_covering(X)[U, V]) | ||
| psi = OOX(UU, U) # Needs to exist by the checks of is_open_func, even though | ||
| # in general UU ⊂ U! | ||
| return hom(MV, MU, [sum([psi(A[i, j]) * MU[j] for j in 1:ngens(MU)]) for i in 1:ngens(MV)], rho) |
There was a problem hiding this comment.
This line will throw an error if ngens(MU) is zero, e.g. for the zero module. And maybe it is not type stable.
The reason is that the empty sum is not well defined, because julia does not know which zero element to choose.
You can fix this by providing the keyword init=zero(MU) .
| # First the easy case: Inheritance from an ancestor in the tree. | ||
| if ambient_scheme(U) === V | ||
| # If the restriction was more complicated than what follows, then | ||
| # it would have been cached earlier and this call would not have happened |
There was a problem hiding this comment.
Relying on caching seems brittle to me. It is necessary?
There was a problem hiding this comment.
Why brittle?
First of all, it is default-stable in the sense that it doesn't produce wrong results in case nothing was cached.
Second, the call to the production_func and restriction_func follows a clear protocol (not yet publicly documented, admittedly) which says that things are first looked up in the caches and only then produced if necessary.
The idea is to use this with ideal sheaves or coherent sheaves which happen to be locally free: We want to ask for things like locally_trivializing_cover or local_complete_intersection_cover. These have to be implemented in a way so that
- the trivializing patches are
PrincipalOpenSubsets orSimplifiedSpecs (or position themselves by other means in the tree-structure) with the results in the desired form on these patches - the restriction maps from their parent nodes are also in the prescribed form and, necessarily, cached.
Then, for every other patch W which sits below a trivializing patch U, we expect to also find restrictions which take into account the intermediate trivializations. That is also the reason why we go up recursively along the tree structure for the productions and restrictions (see @afkafkafk13 's comment above) and not straight to the top.
Does this rule out your concerns?
There was a problem hiding this comment.
Brittle in the sense that a change of the caching behavior might affect this function and this could be unexpected.
But if you say that it is stable by default, I am okay.
For larger computations one might want to clear the caches from time to time / or allow them to be collected / or disable caching (where feasible) to save memory.
There was a problem hiding this comment.
Changing of the caching behaviour will certainly affect this function and whoever wants to do such things in obligated to also clean up here.
Turning off caching is not an option here: We need the stability to have the very same object in memory whenever we call M(U) for a fixed U and a coherent sheaf M in order to maintain the element-parent structure in Oscar.
I put a lot of effort into making the caching garbage collectable in the future. However, that still relies on introducing the WeakKeyIdDicts in Oscar at some point: JuliaCollections/DataStructures.jl#402. I hope this will help with your concerns in this regard.
| # Since we are messing with the internals of the sheaf, we need | ||
| # to leave everything clean. This includes manual caching. | ||
| add_incoming_restriction!(M, U, F, res) | ||
| object_cache(M)[V] = F | ||
| set_attribute!(F, :_presentation_matrix, Asub) |
There was a problem hiding this comment.
@simonbrandhorst : This is an example of where one manually constructs restrictions and is responsible for their cachings.
| dom_UV, dom_res = change_base_ring(OOX(U, UV), domain(M)(U)) | ||
| add_incoming_restriction!(domain(M), U, dom_UV, dom_res) | ||
| add_incoming_restriction!(domain(M), W, dom_UV, | ||
| compose(domain(M)(W, U), dom_res)) | ||
| object_cache(domain(M))[UV] = dom_UV | ||
|
|
||
| cod_UV, cod_res = change_base_ring(OOX(V, UV), codomain(M)(V)) | ||
| add_incoming_restriction!(codomain(M), V, cod_UV, cod_res) | ||
| add_incoming_restriction!(codomain(M), W, dom_UV, | ||
| compose(codomain(M)(W, V), cod_res)) | ||
| object_cache(codomain(M))[UV] = cod_UV |
There was a problem hiding this comment.
@simonbrandhorst : This is another instance of where one needs to manually construct restrictions and, hence, also needs to take care of the caching.
|
Sorry about this morning's work almost doubling the size of the PR. Maybe, you want to have a look again. I will not make any further changes now, except repairing the tests if necessary. |
|
One loose end that we need to pick up here is the following. When creating the refinements, I do not yet fill out the glueings. The function for that has an empty body. We could do this with For the time being, the refinement coverings just skip the glueings and that doesn't break anything, yet. But we should think about these issues. |
| return matrix(map(presentation(M), 1)) | ||
| end | ||
|
|
||
| function fill_with_lazy_glueings!(C::Covering, X::AbsCoveredScheme) |
There was a problem hiding this comment.
This is the function I was talking about.
* clean up ideal sheaves. * First round cleaning up restrictions of coherent sheaves. * Add the remaining methods for the restriction functions. * Add sample implementation for trivializations of coherent sheaves. * Implement trivializations of HomSheaf. Co-authored-by: HechtiDerLachs <zach@mathematik.uni-kl.de>
Continuation of #1846 .