Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Formatted Remaining 22 Files #4161

Merged
merged 3 commits into from
Oct 8, 2020
Merged

Conversation

Yash1256
Copy link
Contributor

@Yash1256 Yash1256 commented Oct 7, 2020

In Issue #4109 Formatted 22 Remaining Files

  • pymc3/step_methods/hmc/quadpotential.py
  • pymc3/tests/test_examples.py
  • pymc3/step_methods/metropolis.py
  • pymc3/tests/test_posteriors.py
  • pymc3/tests/test_ndarray_backend.py
  • pymc3/tests/test_shared.py
  • pymc3/tests/test_special_functions.py
  • pymc3/tests/test_model.py
  • pymc3/model.py
  • pymc3/tests/test_random.py
  • pymc3/tests/test_sqlite_backend.py
  • pymc3/tests/test_starting.py
  • pymc3/tuning/scaling.py
  • pymc3/variational/callbacks.py
  • pymc3/tests/test_theanof.py
  • pymc3/util.py
  • pymc3/variational/operators.py
  • pymc3/theanof.py
  • pymc3/distributions/continuous.py
  • pymc3/tests/test_transforms.py
  • pymc3/variational/opvi.py
  • pymc3/tests/test_distributions.py

@Yash1256
Copy link
Contributor Author

Yash1256 commented Oct 7, 2020

@AlexAndorra and @MarcoGorelli Please Review.

@codecov
Copy link

codecov bot commented Oct 7, 2020

Codecov Report

Merging #4161 into master will not change coverage.
The diff coverage is 66.27%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #4161   +/-   ##
=======================================
  Coverage   88.75%   88.75%           
=======================================
  Files          89       89           
  Lines       14039    14039           
=======================================
  Hits        12461    12461           
  Misses       1578     1578           
Impacted Files Coverage Δ
pymc3/distributions/continuous.py 92.93% <ø> (ø)
pymc3/model.py 89.33% <0.00%> (ø)
pymc3/step_methods/metropolis.py 88.27% <0.00%> (ø)
pymc3/step_methods/hmc/quadpotential.py 79.56% <47.22%> (ø)
pymc3/variational/operators.py 92.68% <75.00%> (ø)
pymc3/theanof.py 90.00% <79.31%> (ø)
pymc3/tuning/scaling.py 58.18% <100.00%> (ø)
pymc3/util.py 94.38% <100.00%> (ø)
pymc3/variational/callbacks.py 95.91% <100.00%> (ø)
pymc3/variational/opvi.py 85.32% <100.00%> (ø)
... and 15 more

Copy link
Contributor

@MarcoGorelli MarcoGorelli left a comment

Choose a reason for hiding this comment

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

Nice! Just have a few minor comments. One involves slightly rewriting a test so that black will keep each test case on one line

pymc3/theanof.py Outdated
@@ -368,8 +367,7 @@ def set_default(self, value):
t1 = (False,) * value.ndim
t2 = self.generator.tensortype.broadcastable
if not t1 == t2:
raise ValueError('Default value should have the '
'same type as generator')
raise ValueError("Default value should have the " "same type as generator")
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
raise ValueError("Default value should have the " "same type as generator")
raise ValueError("Default value should have the same type as generator")

' is zero.'.format(*name_slc[ii]))
raise ValueError('\n'.join(errmsg))
errmsg.append(
"The derivative of RV `{}`.ravel()[{}]" " is zero.".format(*name_slc[ii])
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"The derivative of RV `{}`.ravel()[{}]" " is zero.".format(*name_slc[ii])
"The derivative of RV `{}`.ravel()[{}] is zero.".format(*name_slc[ii])

' is non-finite.'.format(*name_slc[ii]))
raise ValueError('\n'.join(errmsg))
errmsg.append(
"The derivative of RV `{}`.ravel()[{}]" " is non-finite.".format(*name_slc[ii])
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"The derivative of RV `{}`.ravel()[{}]" " is non-finite.".format(*name_slc[ii])
"The derivative of RV `{}`.ravel()[{}] is non-finite.".format(*name_slc[ii])

Comment on lines 809 to 822
(
None,
0.5,
None,
None,
"Incompatible parametrization. Must specify either alpha or n.",
),
(
None,
None,
None,
None,
"Incompatible parametrization. Must specify either alpha or n.",
),
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you change L834 so that it reads

with pytest.raises(ValueError, match=f"Incompatible parametrization. {expected}"):

and then "Incompatible parametrization." can be removed from here (and from the remaining tuples from this particular test):

(None, None, None, None, "Must specify either alpha or n."),

and black will keep this on one line.

You'll then need to run pytest pymc3/tests/test_distributions.py -k test_negative_binomial_init_fail to check that this still passes

If you decide to do this, please comment in the PR description that you've made this change. In general it's good to keep formatting separate from changing code (this way it's easier to review / revert), but IMO here it's a really simple change and it allows the formatting to be applied more cleanly so it's OK

@Yash1256
Copy link
Contributor Author

Yash1256 commented Oct 8, 2020

@MarcoGorelli Please Review

Copy link
Contributor

@MarcoGorelli MarcoGorelli left a comment

Choose a reason for hiding this comment

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

Thanks, looks good to me pending green!

Copy link
Contributor

@AlexAndorra AlexAndorra left a comment

Choose a reason for hiding this comment

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

All good, thanks @Yash1256 !

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.

3 participants