Finish restructuring the tests to follow the structure of the code

[Armavica]

What is this PR about? This PR restructures the last test files to follow the structure of the code, as discussed in #5777.

I think that this closes #5777. One last unresolved question is whether we want to move tests out of pymc, like it is done in aesara for example.

Checklist

• [x] Explain important implementation details 👆
• [x] Make sure that the pre-commit linting/style checks pass.
• [x] Link relevant issues (preferably in nice commit messages)
• [x] Are the changes covered by tests and docstrings?
• [x] Fill out the short summary sections 👇

Major / Breaking Changes

• ...

Bugfixes / New features

• ...

Docs / Maintenance

• Finish restructuring the tests to follow the structure of the code

[codecov[bot]]

Codecov Report

Merging #6125 (a9eca4a) into main (244c37d) will increase coverage by 0.33%. The diff coverage is 99.77%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6125      +/-   ##
==========================================
+ Coverage   93.04%   93.37%   +0.33%     
==========================================
  Files          91      100       +9     
  Lines       20813    21896    +1083     
==========================================
+ Hits        19365    20445    +1080     
- Misses       1448     1451       +3     

Impacted Files	Coverage Δ
pymc/tests/smc/test_smc.py	100.00% <ø> (ø)
pymc/tests/variational/test_inference.py	99.02% <99.02%> (ø)
pymc/tests/step_methods/hmc/test_hmc.py	98.14% <100.00%> (ø)
pymc/tests/step_methods/hmc/test_nuts.py	100.00% <100.00%> (ø)
pymc/tests/step_methods/test_compound.py	100.00% <100.00%> (ø)
pymc/tests/step_methods/test_metropolis.py	100.00% <100.00%> (ø)
pymc/tests/step_methods/test_slicer.py	100.00% <100.00%> (ø)
pymc/tests/variational/test_approximations.py	100.00% <100.00%> (ø)
pymc/tests/variational/test_callbacks.py	100.00% <100.00%> (ø)
pymc/tests/variational/test_opvi.py	100.00% <100.00%> (ø)
... and 3 more

[Armavica]

These metropolis tests are flaky even though they are supposed to be SeededTests. It is because _get_seeds_per_chain creates a numpy.random.default_rng(None) which is based on entropy, not on the global numpy random generator: https://github.com/pymc-devs/pymc/blob/4ea8ddee7b55ccaadeec2549bb8c65617e032453/pymc/sampling.py#L287-L290

My suggestion is to use a seed from the global numpy random generator in case number 4 described in this function's docstring: this makes these tests deterministic. Concretely I would add this couple of lines before L287 above:

if random_state is None:
    random_state = np.random.randint(2**30)

Would that be okay?

[ricardoV94]

No, we officially don't allow users to control seeding via global seeding anymore (even if we still use global seeding internally), because those have less quality.

The solution is to explicitly pass random_seed to the sampling routines in those tests.

But how often does it fail? I haven't seen it fail before for a long time now.

[Armavica]

I see, that makes sense. I will change the name of the class and we will see if that repeats.

But doesn't this mean that SeededTest in its current form is not doing much?

[ricardoV94]

I see, that makes sense. I will change the name of the class and we will see if that repeats.

But doesn't this mean that SeededTest in its current form is not doing much?

Yes it's not doing much anymore, other than providing a seed when requested

[Armavica]

These two tests (TestKmeansInducing::test_kmeans (this one is tested on both Windows and Linux on the github runners but fails only on Windows) and TestDEMetropolisZ::test_tuning_reset) have been consistently failing on this PR, even though they pass locally on my computer. I will check if I did something wrong.

[ricardoV94]

@Armavica Do you want to try to rebase this from main and see if we can get it merged quickly? One of the flaky tests you mention was fixed recently

[ricardoV94]

Otherwise it might be easier to do it one module at a time

[Armavica]

The TestDEMetropolisZ::test_tuning_reset does seem to fail about 10-20% of the time on my laptop. Should I just add a seed?

[ricardoV94]

The TestDEMetropolisZ::test_tuning_reset does seem to fail about 10-20% of the time on my laptop. Should I just add a seed?

Yes

[ricardoV94]

Awesome work @Armavica, are we done?

[Armavica]

Awesome work @Armavica, are we done?

Yes, that was the last batch!