Skip to content

Conversation

@HechtiDerLachs
Copy link
Collaborator

@HechtiDerLachs HechtiDerLachs commented Jun 3, 2025

This PR contains my recent work on speeding up computations of spectral sequences in Cech cohomology; see the accompanying preprint on the ArXiv.

I make this a draft for the beginning and clean everything up, including documentation.

TODOs:

  • Link the article here once it appears on the ArXiv.
  • Add documentation and examples.

@HechtiDerLachs HechtiDerLachs marked this pull request as draft June 3, 2025 09:49
@codecov
Copy link

codecov bot commented Jun 3, 2025

Codecov Report

❌ Patch coverage is 84.81625% with 157 lines in your changes missing coverage. Please review.
✅ Project coverage is 84.88%. Comparing base (c09a2e1) to head (48897a8).
⚠️ Report is 193 commits behind head on master.

Files with missing lines Patch % Lines
experimental/Schemes/src/DerivedPushforward.jl 75.61% 59 Missing ⚠️
experimental/Schemes/src/SpectralSequences.jl 87.80% 55 Missing ⚠️
...bleAndHyperComplexes/src/Morphisms/induced_homs.jl 50.00% 34 Missing ⚠️
src/Modules/Iterators.jl 90.90% 4 Missing ⚠️
...dHyperComplexes/src/Morphisms/SummandProjection.jl 93.75% 2 Missing ⚠️
...perComplexes/src/Morphisms/new_koszul_complexes.jl 96.55% 1 Missing ⚠️
...ndHyperComplexes/src/Morphisms/strand_morphisms.jl 97.36% 1 Missing ⚠️
...l/DoubleAndHyperComplexes/src/Morphisms/strands.jl 95.23% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4951      +/-   ##
==========================================
+ Coverage   84.81%   84.88%   +0.07%     
==========================================
  Files         698      707       +9     
  Lines       94290    95347    +1057     
==========================================
+ Hits        79972    80937     +965     
- Misses      14318    14410      +92     
Files with missing lines Coverage Δ
...tal/DoubleAndHyperComplexes/src/Morphisms/Types.jl 81.42% <100.00%> (+4.23%) ⬆️
...dHyperComplexes/src/Morphisms/shifted_complexes.jl 100.00% <100.00%> (ø)
...perComplexes/src/Morphisms/strand_functionality.jl 100.00% <100.00%> (ø)
...eAndHyperComplexes/src/Morphisms/tensor_product.jl 100.00% <100.00%> (ø)
...AndHyperComplexes/src/Morphisms/total_complexes.jl 100.00% <100.00%> (ø)
...tal/DoubleAndHyperComplexes/src/Morphisms/views.jl 100.00% <100.00%> (ø)
...oubleAndHyperComplexes/src/Objects/Constructors.jl 95.23% <100.00%> (ø)
...DoubleAndHyperComplexes/src/Objects/induced_ENC.jl 96.42% <ø> (ø)
...HyperComplexes/src/Objects/new_koszul_complexes.jl 100.00% <100.00%> (+6.81%) ⬆️
...perComplexes/src/Morphisms/new_koszul_complexes.jl 96.55% <96.55%> (ø)
... and 7 more

... and 25 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@HechtiDerLachs HechtiDerLachs marked this pull request as ready for review June 11, 2025 09:46
@fingolfin fingolfin closed this Jun 18, 2025
@fingolfin fingolfin reopened this Jun 18, 2025
@HechtiDerLachs HechtiDerLachs force-pushed the towards_spectral_sequences branch from 79291b2 to 2e9958d Compare June 24, 2025 09:19
@jankoboehm
Copy link
Contributor

I am ok with it. Tests failing?

@HechtiDerLachs
Copy link
Collaborator Author

Thanks for having a look. This is still awaiting merge of #4955 and rebase on that.

@HechtiDerLachs
Copy link
Collaborator Author

All failing tests are now because of the non-supported syntax findfirst(bla(x) for x in ...) in julia 1.6. Since there is an ongoing discussion about dropping support for the latter, I will not adjust the code here for the moment.

@HechtiDerLachs
Copy link
Collaborator Author

@benlorenz : Could we have a look together at some point today to see what's actually causing the failure on 1.6? I can not reproduce the error here.

@benlorenz
Copy link
Member

1.6 seems to be working after the latest change, 1.10 and 1.12 are failing with some index out of bounds errors in the spectral sequences tests? (https://github.com/oscar-system/Oscar.jl/actions/runs/15959406222/job/45009887648?pr=4951#step:11:3241)

@HechtiDerLachs
Copy link
Collaborator Author

HechtiDerLachs commented Jun 30, 2025

Yes, but I suppose these "index out of bounds" errors are secondary to some findfirst(...) not returning the correct result. Or do you think differently? The same tests pass just fine on my local machine with julia 1.11.1.

Edit: I guess, what I'm asking is that if you, as one of the people who probably see the most failing tests, have any list of buzzwords, like e.g. the findfirst with the unsupported syntax, that we could search for in the contents of this PR, then adjusting these could probably already solve the problem. Of course, I can not ask you to dive into the internals and start debugging with or even for me.

@HechtiDerLachs
Copy link
Collaborator Author

Just a heads up: Yesterday we found that the generic code for computing spectral sequences can not (yet) be applied in the toric setup in its full generality. We have to overhaul that. However, since we are actively working on this together with Friedemann Groh (external), @HereAround, and @emikelsons, I found it in the interest of the Oscar project to get a preliminary version in, so that, for instance, we can continue working on #5032 to eventually resolve the issues. The caveats for the toric code have a temporary safeguard built in by throwing an error, unless the user manually sets a "global exponent vector" to compute with in the toric context. This should leave us ample room for further experiments with Friedemann's code while, at the same time, not giving wrong functionality to the user.

Comment on lines 236 to 241
while !(ind in rngs[cur_rng_ind])
cur_rng_ind+=1
end
k = cur_rng_ind # findfirst(ind in r for r in rngs)
# manual way to project (saves allocations)
vv = FreeModElem(coordinates(v)[rngs[cur_rng_ind]], summands[cur_rng_ind]) #canonical_projection(dom, k)(v)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should work also in 1.6? (I didn't test the code)

Suggested change
while !(ind in rngs[cur_rng_ind])
cur_rng_ind+=1
end
k = cur_rng_ind # findfirst(ind in r for r in rngs)
# manual way to project (saves allocations)
vv = FreeModElem(coordinates(v)[rngs[cur_rng_ind]], summands[cur_rng_ind]) #canonical_projection(dom, k)(v)
k = findfirst(r -> ind in r, rngs)
# manual way to project (saves allocations)
vv = FreeModElem(coordinates(v)[rngs[k]], summands[k]) #canonical_projection(dom, k)(v)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for having a look! However, the point here was not to start looking for k from scratch every time, since the ind come in in an ordered way. Note that cur_rng_ind is another running variable in this loop. This was actually crucial for speed at some point (O(n) vs. O(n^2)).

Copy link
Collaborator Author

@HechtiDerLachs HechtiDerLachs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just some cleanup of the code.

Comment on lines 236 to 241
while !(ind in rngs[cur_rng_ind])
cur_rng_ind+=1
end
k = cur_rng_ind # findfirst(ind in r for r in rngs)
# manual way to project (saves allocations)
vv = FreeModElem(coordinates(v)[rngs[cur_rng_ind]], summands[cur_rng_ind]) #canonical_projection(dom, k)(v)
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for having a look! However, the point here was not to start looking for k from scratch every time, since the ind come in in an ordered way. Note that cur_rng_ind is another running variable in this loop. This was actually crucial for speed at some point (O(n) vs. O(n^2)).

Co-authored-by: Max Horn <[email protected]>
@jankoboehm
Copy link
Contributor

@simonbrandhorst will take another look

@simonbrandhorst simonbrandhorst merged commit e5cef2e into oscar-system:master Jul 2, 2025
35 of 36 checks passed
@HechtiDerLachs HechtiDerLachs deleted the towards_spectral_sequences branch July 2, 2025 12:27
@lgoettgens
Copy link
Member

This is currently mentioned in the Other changes part of the release notes. Can someone (@HechtiDerLachs @simonbrandhorst @jankoboehm) please add one of the following labels to choose in which section it should appear instead? topic: commutative algebra, topic: algebraic geometry, experimental

@lgoettgens
Copy link
Member

This is currently mentioned in the Other changes part of the release notes. Can someone (@HechtiDerLachs @simonbrandhorst @jankoboehm) please add one of the following labels to choose in which section it should appear instead? topic: commutative algebra, topic: algebraic geometry, experimental

I now picked topic: algebraic geometry. If someone disagrees with this, please just change the labels.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

release notes: use title For PRs: the title of this PR is suitable for direct use in the release notes topic: algebraic geometry

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants