Skip to content

Fix tensor_product for SubquoModules#4810

Merged
joschmitt merged 15 commits intooscar-system:masterfrom
HechtiDerLachs:tensor_product_hotfix
May 16, 2025
Merged

Fix tensor_product for SubquoModules#4810
joschmitt merged 15 commits intooscar-system:masterfrom
HechtiDerLachs:tensor_product_hotfix

Conversation

@HechtiDerLachs
Copy link
Copy Markdown
Collaborator

@HechtiDerLachs HechtiDerLachs commented Apr 17, 2025

A hotfix for #4809. Closes #4812.

@lgoettgens lgoettgens changed the title Tensor product hotfix Fix tensor_product for SubquoModules Apr 17, 2025
@lgoettgens lgoettgens added topic: commutative algebra release notes: use title For PRs: the title of this PR is suitable for direct use in the release notes labels Apr 17, 2025
@HechtiDerLachs
Copy link
Copy Markdown
Collaborator Author

Looks to me like the tests for the modules now become too big with these changes. I.e. they do not finish in due time. Is this correct? @aaruni96 ?

@fingolfin fingolfin closed this Apr 30, 2025
@fingolfin fingolfin reopened this Apr 30, 2025
@fingolfin
Copy link
Copy Markdown
Member

@HechtiDerLachs so @aaruni96 is travelling and so may not reply too soon... but say his answer is "yes, too big", what's the plan?

@fingolfin
Copy link
Copy Markdown
Member

@HechtiDerLachs there are also a ton of doctest failures here (so doctest fix needed?)

@HechtiDerLachs HechtiDerLachs force-pushed the tensor_product_hotfix branch from 324fc10 to 1606254 Compare May 15, 2025 11:43
@HechtiDerLachs
Copy link
Copy Markdown
Collaborator Author

HechtiDerLachs commented May 15, 2025

@lgoettgens : Can you tell me how to find out on which testset, possibly even which line the timeouts occur? The output of the failing tests seems difficult to decipher for me. If it is even shown here on GH! Sometimes it's just a black screen for some reason...

Timeouts did not occur for me locally before; at least not for the Schemes tests. So I'm wondering what's causing this.

Edit: Locally I get

julia> include("test/AlgebraicGeometry/Schemes/EllipticSurface.jl")
Test Summary:     | Pass  Total     Time
elliptic surfaces |    9      9  7m24.4s
Test Summary:                                  | Pass  Total  Time
normalize_quartic and transform_to_weierstrass |    2      2  1.6s
Test Summary:                                                       | Pass  Total   Time
normalize_quartic and transform_to_weierstrass over function fields |    2      2  23.4s
Test Summary:      | Total   Time
two neighbor steps |     0  24.4s
Test Summary:           | Pass  Total     Time
moebius transformations |   50     50  2m11.1s
Test.DefaultTestSet("moebius transformations", Any[], 50, false, false, true, 1.747317902545061e9, 1.747318033668844e9, false, "/home/matthias/Oscar.jl/test/AlgebraicGeometry/Schemes/EllipticSurface.jl")

@aaruni96
Copy link
Copy Markdown
Member

The timeout occurs whenever the running time of the test exceeds the time we have allocated for the test (currently 60 minutes for most tests). So, checking the code for "where does timeout occur" is fruitless. Instead, we can think about whether anything can be done (in your changes) to push the time back under 60 minutes, or raise the timeout limit.

Looking at the logs, the best I can tell, is that the tests in Oscar.jl/test/Modules/UngradedModules.jl are ongoing at the time process is killed because of timeout.

@codecov
Copy link
Copy Markdown

codecov Bot commented May 15, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 84.87%. Comparing base (08c390a) to head (4b62d36).
Report is 1 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4810      +/-   ##
==========================================
- Coverage   84.87%   84.87%   -0.01%     
==========================================
  Files         683      683              
  Lines       91746    91783      +37     
==========================================
+ Hits        77872    77898      +26     
- Misses      13874    13885      +11     
Files with missing lines Coverage Δ
...ndHyperComplexes/src/Morphisms/free_resolutions.jl 98.80% <100.00%> (+0.12%) ⬆️
...DoubleAndHyperComplexes/src/Objects/induced_ENC.jl 97.22% <100.00%> (+0.61%) ⬆️
...leAndHyperComplexes/src/Objects/tensor_products.jl 90.50% <ø> (-0.06%) ⬇️
src/Modules/UngradedModules/HomologicalAlgebra.jl 93.08% <100.00%> (+0.26%) ⬆️
src/Modules/UngradedModules/Tensor.jl 97.56% <100.00%> (+2.56%) ⬆️

... and 4 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
Copy link
Copy Markdown
Collaborator Author

Good to go from my side. Waiting for approval and merge. @jankoboehm ?

@HechtiDerLachs HechtiDerLachs mentioned this pull request May 16, 2025
10 tasks
@jankoboehm
Copy link
Copy Markdown
Contributor

Looks good, thank you very much! Eventually we could perhaps shorten the test still a bit more.

@HechtiDerLachs HechtiDerLachs requested a review from jankoboehm May 16, 2025 13:04
@joschmitt joschmitt merged commit fba692f into oscar-system:master May 16, 2025
29 of 35 checks passed
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: commutative algebra

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants