-
-
Notifications
You must be signed in to change notification settings - Fork 528
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
Add sheaf cohomology for projective schemes #37477
base: develop
Are you sure you want to change the base?
Conversation
e94d684
to
c8996fb
Compare
I can review this when its ready. |
I made this PR somewhat prematurely, to test the new PR template. I will soon update and make it ready. Thanks! |
4c27758
to
3cf1833
Compare
3cf1833
to
d11f801
Compare
82fcb58
to
4fd5432
Compare
Documentation preview for this PR (built with commit 2e47081; changes) is ready! 🎉 |
Thinking a bit more, I am wondering if your implementation is the right way to do it. Mathematically, it seems like you should have two separate complexes, which then For anything you want to make internal, you should start with the leading Can you also fix the linter issue too? |
There are other methods to compute the cohomology (the dimension of the cohomology group) of coherent sheaves. Actually Maruyama's method is not the usual one implemented. Eisenbud-Floystad-Schreyer method is used in Macaulay2, and perhaps in Magma. In the future, someone may implement other methods. Perhaps I won't. Then the main public API Hence the module
Now there remains the question of how much "hidden" my implementation of Maruyama's method should be. I thought that declaring the module
OK. I will do these later. |
It’s okay to have the class names be public (I would tend to say this is a good thing for most cases, this one included). However, you can directly access it by public methods (here, |
There is no such method. |
Thanks. Done. |
Sorry, I think I misread a few things while going over the code quickly. Yes, However, I am fairly convinced the entire design should be reconsidered as the two separate complexes for the top and bottom complexes, both of which are public facing that could be done by a single class (differing by a single input parameter). So we would need the appropriate comparisons (where If you don’t want to change the design, would you be open to me rewriting it with this approach as a replacement PR? |
I know that you have a good intention. Thanks for that. But I don't understand your proposal. It just sounds like overkill, like coating a break pedal with platinum (I just made up this analogy). I suspect that you are overzealous :-) I do appreciate your attention and time in reviewing this PR. Please don't get me wrong. But let's solve the situation constructively. How about taking one of these two actions: (A) You accept this PR as it is. Then you author a subsequent PR that implements your design. I may review the PR, but cannot be sure now that I would be positive to it. (B) Or you leave this PR, and I will wait for a new reviewer. ? |
I assume that you chose (B). Thanks for the review! |
bf276c9
to
a2447bc
Compare
a2447bc
to
5ddb759
Compare
Regarding the code duplication that Travis pointed out, I think I may combine methods of class
This looks a bit ugly, but really removes a nontrivial code duplication. Let me know if this is desirable. |
Better not to do the refactoring suggested in #37477 (comment), since Travis authored a subsequent PR #38003 that does more extensive refactoring based on the present PR. |
Okay, I finally found the time to do my proposed rewrite of this on #38003 as @kwankyu stated. In there, I provide the implementation I was recommending. In my proposal, everything is exposed to the user, it uses caching and unique representation, removes redundant code and documentation by making top-versus-bottom a boolean on each class, and separates the top versus bottom complexes with a class that models the entire cohomology. I believe this provides several distinct advantages:
The bulk of the code for #38003 is based off this branch. |
OK. I regard #38003 as a sequel to this PR. Please copy and paste your remarks above into the description of your PR. |
4bed7d2
to
6976521
Compare
rebased. |
As far as I see it, this PR has no problem, except some repetitive code (small to me, not so small to Travis). I want rather to keep the repetitive part for readibility while Travis wants to remove the repetitive part and refactor the code structure as he sees fit. So I regard #38003 as sequel to this PR. Actually #38003 should set this PR as dependency. I may review #38003 after this PR.
No. There is no "experimental" code. |
It isn't a sequel though, it is an alternative proposal. I have reservations about including the code as-is. You are asking me to include your code (despite my objections) without any guarantees that my code will not be opposed. |
That was my interpretation of your saying "The bulk of the code for #38003 is based off this branch". I misunderstood and I am sorry. I now see your intention.
I don't oppose a PR as a reviewer. I never did. If I don't like a PR, I leave it to others. I suggest my ideas to improve the code, but it is all up to the author whether to accept them or not. Actually I did not see your PR yet. I don't know whether I will be positive or just leave. (By the way, my "positive over leave" ratio is very high. Matthias will agree :-) |
As you left this comment here:
For the purpose, I exposed
Note that the paper does not construct a vector space isomorphic to the 0-th cohomology group. |
9e99c6c
to
d7df283
Compare
d7df283
to
a8c6abb
Compare
a8c6abb
to
b998e91
Compare
cd76d09
to
5866428
Compare
Co-authored-by: Matthias Köppe <[email protected]>
0db5fdc
to
1f5b27b
Compare
so we add a sheaf cohomology machinery of coherent sheaves on projective schemes. Using the machinery, we implement some invariants for projective subschemes. In particular,
📝 Checklist
⌛ Dependencies