sagemath / sage

Main repository of SageMath
https://www.sagemath.org
Other
1.3k stars 447 forks source link

Long testing time: topology/moment_angle_complex.py #37378

Closed gmou3 closed 5 months ago

gmou3 commented 6 months ago

There are quite a few time-consuming tests in this file, most of them without a # long time designation.

A couple of them are especially bad:

File "src/sage/topology/moment_angle_complex.py", line 595, in sage.topology.moment_angle_complex.MomentAngleComplex.?
Warning, slow doctest:
    Z.homology()
Test ran for 63.65 s, check ran for 0.00 s
[...]
File "src/sage/topology/moment_angle_complex.py", line 721, in sage.topology.moment_angle_complex.MomentAngleComplex.euler_characteristic
Warning, slow doctest:
    M.euler_characteristic()
Test ran for 54.54 s, check ran for 0.00 s

@OP5642 Can these two tests be replaced with faster ones from smaller simplicial complexes?

tscrim commented 6 months ago

Those tests are far faster on my laptop (~16s and ~11s), but yes, they should have been marked as # long time (which was my fault as the reviewer). However, it is not easier to make them smaller and nontrivial. Indeed, part of the point is they are (very) large simplicial complexes that are computed within a reasonable time. I think we should just mark them as # long time rather than try to shrink them down.

gmou3 commented 6 months ago

Ok. On a more general point, why do we always test these # long time commands? Shouldn't this only happen manually or at specific points of the release cycle?

OP5642 commented 6 months ago

I apologize for not addressing this sooner, but yes, as @tscrim said, I think it's best to mark them as # long time, because we want to show that the computation is relatively quick even for large simplicial complexes.

tscrim commented 6 months ago

Ok. On a more general point, why do we always test these # long time commands? Shouldn't this only happen manually or at specific points of the release cycle?

They have been known to catch bugs that cannot be seen on smaller examples. So it is good to run them through the CI tests. They are also tested before each PR is merged (for the same reason).

gmou3 commented 6 months ago

They are also tested before each PR is merged (for the same reason).

That's reasonable. My puzzlement is about the fact that they are run after each commit in a PR: doesn't that make the # long time labels less purposeful?

tscrim commented 6 months ago

They are also tested before each PR is merged (for the same reason).

That's reasonable. My puzzlement is about the fact that they are run after each commit in a PR: doesn't that make the # long time labels less purposeful?

For CI testing, yes, but not for testing locally on a developer’s machine.

gmou3 commented 6 months ago

Ok, I see. Thank you.