Improve performance of strict transform under toric blowups#4484
Merged
HereAround merged 2 commits intooscar-system:masterfrom Jan 18, 2025
Merged
Improve performance of strict transform under toric blowups#4484HereAround merged 2 commits intooscar-system:masterfrom
HereAround merged 2 commits intooscar-system:masterfrom
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #4484 +/- ##
==========================================
+ Coverage 84.37% 84.39% +0.01%
==========================================
Files 672 672
Lines 88717 88690 -27
==========================================
- Hits 74855 74847 -8
+ Misses 13862 13843 -19
|
HereAround
approved these changes
Jan 18, 2025
Member
HereAround
left a comment
There was a problem hiding this comment.
Wow! That is cool! Thank you @paemurru I am very content with your changes!
paemurru
added a commit
to paemurru/Oscar.jl
that referenced
this pull request
Jan 27, 2025
Fixes a bug in FTheoryTools instroduced by oscar-system#4484 Namely, it previously computed the total transform, not the strict transform. Even with the speed improvements of oscar-system#4485 computing the strict transform with ideals is slower than operating on polynomials. Therefore, I added a strict transform function on polynomials.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
I improved the speed of
strict_transformfor an arbitrary toric blowup. More precisely, I improvedcox_ring_module_homomorphism, which is used forstrict_transform,total_transformandstrict_transform_with_index. The main time-saving techniques:MPolyBuildCtxto create the polynomial, instead of iteratively adding terms to a polynomial,Vector{Rational{Int64}}instead ofVector{QQFieldElem}for the vectorps,Vector{Int64}for the vectorpsin the special case where the denominators are 1.Some small other improvements in the core loop:
first(exponents(g))instead ofcollect(exponents(g))[1],exps = [exps; exceptional_exp]instead ofpush!(exps, exceptional_exp)reduces allocated memory by about 35% --- I am puzzled why there is a difference here.The function
strict_transformnow seems to be faster than the specialized_strict_transformin FTheoryTools that could compute strict transform only under blowups along smooth cones. So I removed the specialized function.Also, I added a test for computing the strict transform when blowing up along an existing ray.
Benchmarks
I used the F-Theory computations from @HereAround . Namely, I first commented out lines 797--803 of
experimental/FTheoryTools/src/AbstractFTheoryModels/methods.jland edited line 821 of the same file from
for k in 1:nr_blowupstofor k in 1:5to have a faster test.Then ran
Results
Previously:
Now:
Improvements