Fixup mypy errors in sampling.py #4327
Conversation
pymc3/sampling.py
Outdated
| vars_ = [var.name for var in prior_vars + prior_pred_vars] | ||
| vars = set(vars_) | ||
| vars_: Iterable[str] = [var.name for var in prior_vars + prior_pred_vars] | ||
| elif vars is None: | ||
| vars = var_names | ||
| vars_ = vars | ||
| elif vars is not None: | ||
| assert var_names is not None # help mypy | ||
| vars_ = var_names | ||
| elif var_names is None: | ||
| warnings.warn("vars argument is deprecated in favor of var_names.", DeprecationWarning) | ||
| vars_ = vars | ||
| else: | ||
| raise ValueError("Cannot supply both vars and var_names arguments.") | ||
| vars = cast(TIterable[str], vars) # tell mypy that vars cannot be None here. |
There was a problem hiding this comment.
only vars_ is used after this point, any reason to assign to vars?
There was a problem hiding this comment.
I think the reason is in the first if branch: vars = set(vars_). Looks like the for in Line 1945/1956 is supposed to iterate over each value exactly once.
Codecov Report
@@ Coverage Diff @@
## master #4327 +/- ##
==========================================
+ Coverage 87.66% 87.68% +0.02%
==========================================
Files 88 88
Lines 14264 14240 -24
==========================================
- Hits 12504 12486 -18
+ Misses 1760 1754 -6
|
pymc3/sampling.py
Outdated
| CategoricalGibbsMetropolis, | ||
| PGBART, | ||
| CompoundStep, | ||
| ] |
There was a problem hiding this comment.
Users may implement their own step method. All of these should already inherit from arraystep.BlockedStep.
pymc3/sampling.py
Outdated
| elif vars is not None: | ||
| assert var_names is not None # help mypy | ||
| vars_ = var_names | ||
| elif var_names is None: | ||
| warnings.warn("vars argument is deprecated in favor of var_names.", DeprecationWarning) |
There was a problem hiding this comment.
For how long has this been in? 4.0 is coming up, so we might as well remove that kwarg already. Would make some of this typing easier too.
pymc3/sampling.py
Outdated
| vars_ = [var.name for var in prior_vars + prior_pred_vars] | ||
| vars = set(vars_) | ||
| vars_: Iterable[str] = [var.name for var in prior_vars + prior_pred_vars] | ||
| elif vars is None: | ||
| vars = var_names | ||
| vars_ = vars | ||
| elif vars is not None: | ||
| assert var_names is not None # help mypy | ||
| vars_ = var_names | ||
| elif var_names is None: | ||
| warnings.warn("vars argument is deprecated in favor of var_names.", DeprecationWarning) | ||
| vars_ = vars | ||
| else: | ||
| raise ValueError("Cannot supply both vars and var_names arguments.") | ||
| vars = cast(TIterable[str], vars) # tell mypy that vars cannot be None here. |
There was a problem hiding this comment.
I think the reason is in the first if branch: vars = set(vars_). Looks like the for in Line 1945/1956 is supposed to iterate over each value exactly once.
MarcoGorelli
force-pushed
the
sort-out-typing
branch
from
22d49eb
to
66f4ed6
Compare
|
Thanks @MarcoGorelli! |
I understand there's some disagreement about the usefulness of type hints, so perhaps we don't need to type everything, but I think that at least the types hints that are already there shouldn't throw errors