-
Notifications
You must be signed in to change notification settings - Fork 126
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
Projective modules #1561
Projective modules #1561
Conversation
@afkafkafk13 : Compared to yesterday I had to change the methods for Also, please have a look at the smoothness test and let us know whether you approve it. |
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.
singular locus/ is_smooth needs some more discussion
``σ ∘ π``. | ||
""" | ||
function is_projective(M::SubQuo; check::Bool=true) | ||
### Assemble a presentation of M over its base ring |
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 really a good idea to do the computation of a free presentation/presentation matrix here explicitly instead of having and using a function 'free presentation' which is also user accessible? ( This would leave for this function: check anihilators of M, compute presentation matrix, call _is_projective).
If you only do it by hand to avoid interference with other concurrent developments and plan to restructure later, this is ok, but should be marked in the code.
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.
Agreed. I will add a comment in the code.
end | ||
|
||
function (R::MPolyRing)(a::MPolyLocalizedRingElem; check::Bool=true) | ||
isone(numerator(a)) && return denominator(a) |
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.
Are the roles of numerator and denominator mistakenly switched in this line? You do want to return x for x/1, but not for 1/x.
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.
Yes, this is a mistake.
one(OO(X)) in OO(X)(K) && return result | ||
return [subscheme(X, K)] | ||
else | ||
P = primary_decomposition(saturated_ideal(I)) |
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.
You do not need a full primary decomposition here. An equidimensional decomposition suffices: for schemetheoretic purposes use equidimensional_decomposition_weak, for varietes use equidimensional_decomposition_radical.
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.
Alright, I was not aware of that. Let us discuss this in person.
return subscheme(X, one(OO(X))) | ||
end | ||
R = base_ring(OO(X)) | ||
return Spec(R, prod([modulus(OO(Y)) for Y in comp]), inverted_set(OO(X))) |
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.
This should be intersect not prod. If you are working schemetheoretically, the expected structure is the intersection. If you are working on varieties, you want a reduced structure to be returned, which is not the case with products.
For the case of varieties think of x(x^2-y^3) where we have <x,y> as radical describing the intersection locus of the two branches and <x,y> as radical describing the singular locus of one branch. The result expected by the user is obviously <x,y> and not <x,y>^2.
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.
intersect
is significantly more expensive than taking products. So if people were only interested in the set-theoretic result, they might appreciate the shorter runtime. Maybe this can be resolved by a flag?
return result | ||
end | ||
|
||
@attr Bool function is_equidimensional(X::AbsSpec{<:Ring, <:MPolyQuoLocalizedRing}) |
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.
This should be available in commutative algebra. If it is not available there then move it to the appropriate file in Rings. By the way: only check the number of components in an equidimensional decomposition, this saves a lot of work as compared to primary decomposition!
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.
For moving it to the section on rings, we have to consult with those responsible for the MPolyQuo
s. But maybe, it is already there, indeed.
function _singular_locus_with_decomposition(X::AbsSpec{<:Ring, <:MPolyQuoLocalizedRing}) | ||
I = localized_modulus(OO(X)) | ||
result = typeof(X)[] | ||
if is_equidimensional(X) |
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.
This test only makes sense, if you hand over a flag whenever you are already sure that you are equidimensional. Testing for it is not cheaper than computing the equidimensional decomposition. So you would probably like to add an additional optional argument is_equidim=false for which you test 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.
This is an internal function only to be used with equidimensional input. No flag required.
L == base_ring(J) || error("ideals must be defined in the same ring") | ||
preI = Oscar.pre_image_ideal(I) | ||
preJ = Oscar.pre_image_ideal(J) | ||
R = base_ring(L) |
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.
Why do you define R here without using it?
return parent(f)(divexact(num, g), divexact(den, g), check=false) | ||
end | ||
|
||
function jacobi_matrix(f::MPolyLocalizedRingElem) |
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.
General question on jacobian matrix (boiling down to derivative computation) in localized rings: I agree that the exact functionality needs to be available, but programmer-facing it might be a good idea to also provide this in a more lazy version omitting the denominators from the beginning whenever a suitable flag is set. I am thinking of contexts where subsequent computations will be performed modulo the ideal in question and one does not want to first introduce extra terms just to delete them later on. This might be a flag like forget_denominators=false among the arguments..
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.
No, I clearly object to that. If we wish to do things in localized rings, we need to be sure to have the corresponding elements. Otherwise, it becomes completely messy to write generic code which is parametrized with the type of the ring. If anything, we would have to make elements of localized rings behave like polynomials whenever they effectively are polynomials; e.g. print without denominator, etc. Starting implementations with flags is not user-friendly and breaks type stability. Since addition, multiplication, etc. is critical, type stability is key here.
f = gens(I) | ||
Df = jacobi_matrix(f) | ||
|
||
A = map_entries(x->OO(X)(x), Df) |
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.
Remark referring to a previous comment: Well, this is precisely such an application of the jacobian matrix, where your subsequent computations are modulo the ideal anyway and you can simply forget about the denominators without messing up the result.
…ll primary decomposition
…jl into projective_modules
@HechtiDerLachs: Two things we should discuss outside this chat:
|
I can not really make sense out of the error message from the failure of the Documentation test suite. If anyone recognizes what is going wrong, please let me know. |
The offending file does not even exist in this branch, so I assume that something is broken in master and we see the fallout. There is hope that it will be solved by the time this PR is ready for merging |
This was broken on master and later fixed by #1570. Closing and re-opening this PR will cause the tests to be re-run against latest master, so I'll do that now. |
…jl into projective_modules
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.
Now I am content and the changes may be merged.
For a$M$ over a ring $R$ we provide a test whether or not $M$ is a projective module. In the affirmative case, we return the decomposition $R^r \cong M \oplus N$ for some $r$ and some module $N$ . In particular, this leads to an efficient smoothness test for affine varieties by checking whether or not the cotangent sheaf is locally free.
SubQuo