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

• Explain important implementation details 👆
• Make sure that the pre-commit linting/style checks pass.
• Link relevant issues (preferably in nice commit messages)
• Are the changes covered by tests and docstrings?
• 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]

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:

pymc/pymc/sampling.py

Lines 287 to 290 in 4ea8dde

	if random_state is None or isinstance(random_state, int):
	if chains == 1 and isinstance(random_state, int):
	return (random_state,)
	return _get_unique_seeds_per_chain(np.random.default_rng(random_state).integers)

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

[ricardoV94]

Isn't it enough to do -pymc/tests/step_methods/ ?

[Armavica]

Yes it is, but here I selected only the few files where tests from test_posteriors ended up, to not increase too much the runtime of the tests. The rest is already tested in other platforms. I am not sure how much of a difference that makes, though. If we test the whole of step_methods I will also check that the check_all_tests_are_covered.py script understands what happens.

[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!