Skip to content

Conversation

@chritter
Copy link
Contributor

@chritter chritter commented Mar 4, 2022

Fixes #5515. We have implemented tests for pymc.distributions.logprob.get_scaling which include edge cases and a test with a model graph random variable. Please let us know if the test coverage is sufficient or if further tests are required.

Co-authored-by: @Icyshaman.

Depending on what your PR does, here are a few things you might want to address in the description:

#DataUmbrella Sprint
@reshamas

@codecov
Copy link

codecov bot commented Mar 4, 2022

Codecov Report

Merging #5544 (af2e5f9) into main (53ad689) will decrease coverage by 2.55%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #5544      +/-   ##
==========================================
- Coverage   89.21%   86.66%   -2.56%     
==========================================
  Files          75       75              
  Lines       13846    13846              
==========================================
- Hits        12353    11999     -354     
- Misses       1493     1847     +354     
Impacted Files Coverage Δ
pymc/smc/smc.py 18.47% <0.00%> (-77.92%) ⬇️
pymc/smc/sample_smc.py 16.77% <0.00%> (-66.45%) ⬇️
pymc/distributions/simulator.py 46.52% <0.00%> (-40.98%) ⬇️
pymc/backends/report.py 90.29% <0.00%> (-0.75%) ⬇️
pymc/step_methods/metropolis.py 82.55% <0.00%> (-0.43%) ⬇️
pymc/distributions/logprob.py 97.45% <0.00%> (+0.84%) ⬆️

Copy link
Member

@michaelosthege michaelosthege left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking quite good.
I'm not familiar with _get_scaling so I can't tell if the numbers are correct, but I assume that they are.

Can you add match="part of the message" to the pytest.raises(...)? That'll make sure that it's testing the right errors.

@michaelosthege
Copy link
Member

@ricardoV94 @chritter can you check/fix/apply the suggestions I commented? Let's get this over the finish line

@michaelosthege michaelosthege requested a review from ricardoV94 May 6, 2022 08:03
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
with pytest.raises(TypeError):
with pytest.raises(TypeError, match="expected int or list of ints"):

Comment on lines 56 to 70
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
with pytest.raises(ValueError):
_get_scaling([1, 2, 5, Ellipsis, Ellipsis], (2, 3), 2)
with pytest.raises(ValueError):
_get_scaling([1, 2, 5, Ellipsis], (2, 3), 2)
with pytest.raises(ValueError, match="Double Ellipsis"):
_get_scaling([1, 2, 5, Ellipsis, Ellipsis], (2, 3), 2)
with pytest.raises(ValueError, match="scalings is bigger that ndim"):
_get_scaling([1, 2, 5, Ellipsis], (2, 3), 2)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
with pytest.raises(ValueError):
with pytest.raises(ValueError, match="scalings is bigger that ndim"):

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
with pytest.raises(TypeError):
with pytest.raises(TypeError, match="expected int or list of ints"):

Copy link
Member

@michaelosthege michaelosthege left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I rebased it to fix git conflicts.

Will merge once the tests pass.

@michaelosthege michaelosthege merged commit 49eecfb into pymc-devs:main May 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add test for _get_scaling

2 participants