Mirror codebase structure in tests #6084

Armavica · 2022-08-31T06:55:13Z

What is this PR about?

This PR splits up the test files test_distribution.py, test_moments.py and test_random.py, to start tackling issue #5777.

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

Break up test_distribution.py, test_moments.py and test_random.py in more specific files.

codecov · 2022-08-31T07:15:19Z

Codecov Report

Merging #6084 (32b16fa) into main (0b191ad) will increase coverage by 2.22%.
The diff coverage is 97.61%.

@@            Coverage Diff             @@
##             main    #6084      +/-   ##
==========================================
+ Coverage   89.54%   91.76%   +2.22%     
==========================================
  Files          72       86      +14     
  Lines       12929    17692    +4763     
==========================================
+ Hits        11577    16235    +4658     
- Misses       1352     1457     +105

Impacted Files	Coverage Δ
pymc/tests/distributions/test_dist_math.py	`99.25% <ø> (ø)`
pymc/tests/distributions/test_timeseries.py	`94.06% <85.71%> (ø)`
pymc/tests/distributions/util.py	`91.08% <91.08%> (ø)`
pymc/tests/distributions/test_distribution.py	`97.46% <97.46%> (ø)`
pymc/tests/distributions/test_multivariate.py	`97.73% <97.73%> (ø)`
pymc/tests/distributions/test_discrete.py	`98.87% <98.87%> (ø)`
pymc/tests/distributions/test_continuous.py	`99.76% <99.76%> (ø)`
pymc/distributions/continuous.py	`97.51% <100.00%> (ø)`
pymc/tests/distributions/test_bound.py	`100.00% <100.00%> (ø)`
pymc/tests/distributions/test_censored.py	`100.00% <100.00%> (ø)`
... and 15 more

pymc/tests/distributions/test_distribution.py

ricardoV94 · 2022-09-01T14:03:10Z

This draft PR is a WIP to tackle issue #5777, posted early to avoid duplication of effort.

To avoid conflicts with the other PRs I would suggest we merge this earlier rather than later.

pymc/tests/distributions/test_distribution.py

Armavica · 2022-09-01T16:50:42Z

To avoid conflicts with the other PRs I would suggest we merge this earlier rather than later.

Thank you. I don't mind rebasing, but I see what I can do tonight.

ricardoV94 · 2022-09-01T17:04:19Z

I didn't mean you have to rush like that. Just giving it priority over other PRs.

Armavica · 2022-09-03T06:43:24Z

The files test_distribution.py, test_moment.py and test_random.py are more or less done. I am just not sure what to do with the final test that remains in test_random.py.

I am planning to work on the others this weekend (gp, ode, smc, step_methods, hmc, tuning, variational), but you can merge this one now if you want, it is fine for me either way.

pymc/tests/distributions/test_random.py

pymc/tests/distributions/test_timeseries.py

pymc/tests/distributions/test_distribution.py

ricardoV94 · 2022-09-03T08:41:44Z

I am planning to work on the others this weekend (gp, ode, smc, hmc, tuning, variational), but you can merge this one now if you want, it is fine for me either way.

Yes let's keep this PR just about the distribution tests

ricardoV94 · 2022-09-03T08:43:27Z

Looks nearly done. I left some final suggestions. Once you address them I think we can merge.

ricardoV94 · 2022-09-03T08:52:21Z

Can you add a bullet point in the maintenance section on the top PR post?

Also some tests seem to be failing, let me know if you need help with those (haven't peaked at the logs)

Armavica · 2022-09-03T13:33:03Z

Thank you for your review! Yes, the tests should be fixable. The only one that I don't understand is the docs: it fails to build the 404 page for an obscure reason... Do you have an idea about this? And it was the same error for the first version of the PR.

ricardoV94 · 2022-09-03T14:00:26Z

I think the docs build has been failing everywhere, you can ignore it

Armavica · 2022-09-03T20:16:30Z

I think it is all set now. We should probably think of rebalancing the github test workers at some point, with many tests having moved around.

ricardoV94 · 2022-09-03T20:30:19Z

We should probably think of rebalancing the github test workers at some point, with many tests having moved around.

Yes, after a few more PRs get open/merged we can get an idea of the average runtime to rebalance.

ricardoV94 · 2022-09-03T20:35:08Z

pymc/tests/distributions/test_distribution.py

+                evaled_moment = moment(a).eval({mu: mu_val})
+
+
+def test_all_distributions_have_moments():


This seems to be repeated above

Still looks unresolved on my end? Codecov is complaining the test above are not being covered, which agress with them still being duplicated.

ricardoV94 · 2022-09-03T20:35:39Z

pymc/tests/distributions/test_distribution.py

+        )
+
+
+class TestDensityDist:


There's already a TestDensityDist class above

Still duplicated above?

pymc/tests/distributions/test_timeseries.py

Armavica · 2022-09-03T21:01:26Z

Argh, my bad, tricky rebasing… Good catches.

canyon289 · 2022-09-03T21:18:01Z

@Armavica youre doing an amazing job. Thanks for all the effort

ricardoV94

Still some duplicated tests in test_distributions and an indentation error that is putting two tests inside the previous one. I know how painful rebases can be :D

Otherwise looks great!!!

ricardoV94 · 2022-09-04T14:16:46Z

pymc/tests/distributions/test_continuous.py

+        ),
+    ],
+)
+def test_asymmetriclaplace_moment(b, kappa, mu, size, expected):


This one is missing an indentation, which is causing the following tests to be inside it and not run (codecov warning)

ricardoV94 · 2022-09-04T14:22:06Z

pymc/tests/distributions/test_distribution.py

+                evaled_moment = moment(a).eval({mu: mu_val})
+
+
+def test_all_distributions_have_moments():


Still looks unresolved on my end? Codecov is complaining the test above are not being covered, which agress with them still being duplicated.

ricardoV94 · 2022-09-04T14:22:40Z

pymc/tests/distributions/test_distribution.py

+        )
+
+
+class TestDensityDist:


Still duplicated above?

Armavica · 2022-09-04T15:59:54Z

Ok, I understood my mistake: I think I was repeatedly trying to fix the wrong commit. The rebase went without problem this time so I think that it should be ok. And thank you for mentioning codecov: I understood where to look to spot all of this.

ricardoV94 · 2022-09-04T19:52:07Z

Merged! Thanks for this :) Feel free to break some more eggs.

ricardoV94 added tests maintenance labels Aug 31, 2022

ricardoV94 reviewed Aug 31, 2022

View reviewed changes

pymc/tests/distributions/test_distribution.py Show resolved Hide resolved

ricardoV94 requested changes Sep 1, 2022

View reviewed changes

ricardoV94 reviewed Sep 1, 2022

View reviewed changes

pymc/tests/distributions/test_distribution.py Outdated Show resolved Hide resolved

Armavica force-pushed the structure-tests branch from b9195ef to ff2f817 Compare September 3, 2022 06:36

Armavica force-pushed the structure-tests branch from e44ff1b to 8223642 Compare September 3, 2022 06:59

ricardoV94 requested changes Sep 3, 2022

View reviewed changes

pymc/tests/distributions/test_random.py Outdated Show resolved Hide resolved

pymc/tests/distributions/test_timeseries.py Outdated Show resolved Hide resolved

pymc/tests/distributions/test_distribution.py Outdated Show resolved Hide resolved

Armavica force-pushed the structure-tests branch 2 times, most recently from e0f336f to bf316ac Compare September 3, 2022 19:30

Armavica marked this pull request as ready for review September 3, 2022 20:14

ricardoV94 requested changes Sep 3, 2022

View reviewed changes

Break up test_distributions.py

2e82c1c

Armavica force-pushed the structure-tests branch from bf316ac to 0b86b76 Compare September 3, 2022 21:26

Armavica requested a review from ricardoV94 September 3, 2022 22:11

ricardoV94 requested changes Sep 4, 2022

View reviewed changes

Armavica added 3 commits September 4, 2022 11:50

Break up test_moments.py

144828b

Break up test_random.py

4153762

Remove unused integrate_nd function

32b16fa

Armavica force-pushed the structure-tests branch from 0b86b76 to 32b16fa Compare September 4, 2022 15:54

ricardoV94 approved these changes Sep 4, 2022

View reviewed changes

ricardoV94 merged commit 4c4d71c into pymc-devs:main Sep 4, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Mirror codebase structure in tests #6084

Mirror codebase structure in tests #6084

Armavica commented Aug 31, 2022 •

edited

Loading

codecov bot commented Aug 31, 2022 •

edited

Loading

ricardoV94 commented Sep 1, 2022

Armavica commented Sep 1, 2022

ricardoV94 commented Sep 1, 2022

Armavica commented Sep 3, 2022 •

edited

Loading

ricardoV94 commented Sep 3, 2022 •

edited

Loading

ricardoV94 commented Sep 3, 2022

ricardoV94 commented Sep 3, 2022

Armavica commented Sep 3, 2022 •

edited

Loading

ricardoV94 commented Sep 3, 2022

Armavica commented Sep 3, 2022

ricardoV94 commented Sep 3, 2022

ricardoV94 Sep 3, 2022

ricardoV94 Sep 4, 2022

ricardoV94 Sep 3, 2022

ricardoV94 Sep 4, 2022

Armavica commented Sep 3, 2022

canyon289 commented Sep 3, 2022

ricardoV94 left a comment

ricardoV94 Sep 4, 2022

ricardoV94 Sep 4, 2022

ricardoV94 Sep 4, 2022

Armavica commented Sep 4, 2022

ricardoV94 commented Sep 4, 2022

		evaled_moment = moment(a).eval({mu: mu_val})


		def test_all_distributions_have_moments():

Mirror codebase structure in tests #6084

Mirror codebase structure in tests #6084

Conversation

Armavica commented Aug 31, 2022 • edited Loading

Major / Breaking Changes

Bugfixes / New features

Docs / Maintenance

codecov bot commented Aug 31, 2022 • edited Loading

Codecov Report

ricardoV94 commented Sep 1, 2022

Armavica commented Sep 1, 2022

ricardoV94 commented Sep 1, 2022

Armavica commented Sep 3, 2022 • edited Loading

ricardoV94 commented Sep 3, 2022 • edited Loading

ricardoV94 commented Sep 3, 2022

ricardoV94 commented Sep 3, 2022

Armavica commented Sep 3, 2022 • edited Loading

ricardoV94 commented Sep 3, 2022

Armavica commented Sep 3, 2022

ricardoV94 commented Sep 3, 2022

ricardoV94 Sep 3, 2022

Choose a reason for hiding this comment

ricardoV94 Sep 4, 2022

Choose a reason for hiding this comment

ricardoV94 Sep 3, 2022

Choose a reason for hiding this comment

ricardoV94 Sep 4, 2022

Choose a reason for hiding this comment

Armavica commented Sep 3, 2022

canyon289 commented Sep 3, 2022

ricardoV94 left a comment

Choose a reason for hiding this comment

ricardoV94 Sep 4, 2022

Choose a reason for hiding this comment

ricardoV94 Sep 4, 2022

Choose a reason for hiding this comment

ricardoV94 Sep 4, 2022

Choose a reason for hiding this comment

Armavica commented Sep 4, 2022

ricardoV94 commented Sep 4, 2022

Armavica commented Aug 31, 2022 •

edited

Loading

codecov bot commented Aug 31, 2022 •

edited

Loading

Armavica commented Sep 3, 2022 •

edited

Loading

ricardoV94 commented Sep 3, 2022 •

edited

Loading

Armavica commented Sep 3, 2022 •

edited

Loading