fix: clear the mechanical type errors and shrink the allowlist to one open question - #1148
Open
anevolbap wants to merge 3 commits into
Open
fix: clear the mechanical type errors and shrink the allowlist to one open question#1148anevolbap wants to merge 3 commits into
anevolbap wants to merge 3 commits into
Conversation
drbenvincent
added
review:high
Collaborator
Automated triageRecommendation: Why:
Review focus:
Confidence: high |
`round_num` declared `n: float`, but four call sites pass a zero-dimensional DataArray straight from `.mean()` on posterior samples: regression_kink twice, regression_discontinuity and prepostnegd. That works at runtime, because xarray formats a scalar DataArray like the number it wraps, so this was the annotation being narrower than the function. Widened to `float | xr.DataArray`, matching its sibling `convert_to_string`, which already declared exactly that union. `_format_sig_figs` takes the same value one call deeper and gets the same treatment. Widening a parameter is backwards compatible, so no caller has to change. Clears 4 errors and removes regression_kink, regression_discontinuity and prepostnegd from the allowlist entirely.
Each of these constructors takes `data: DataFrameLike` and then rebinds that same parameter to the pandas frame that `to_pandas` or `to_pandas_with_time_index` returns. mypy keeps the declared parameter type for the rest of the body, so `data.index`, `data.copy()` and `data[column]` were all reported against `NativeDataFrame`, which has none of them. The functions were already annotated `-> pd.DataFrame`; the information was lost at the rebinding, not at the source. The converted frame now gets its own name, `pandas_data`, in synthetic_control, interrupted_time_series, synthetic_difference_in_differences, staggered_did, diff_in_diff and panel_regression. That also matches what the existing comments in those constructors already said, that the conversion returns a copy the experiment owns rather than the caller's frame. Clears 9 errors and empties the `attr-defined` group. Behaviour is unchanged: the same object is stored on `self.data` and passed to validation as before. Full suite: 2284 passed, 18 skipped.
Five unrelated one-liners, each where the annotation and the value already agreed at runtime. `reporting` used `np.abs(effect)` on a DataArray. numpy's stubs say that returns an ndarray, while xarray dispatches it back to a DataArray, which is what `_posterior_probability` wants. The builtin `abs()` goes through `__abs__` and is typed correctly, and computes the same thing. `_arviz_compat` sorted a set of xarray dimension names, which are `Hashable` and not necessarily orderable. The names go straight into an error message, so they sort as strings. `plot_utils._interval_bound_values` bound `lower` and `upper` to arrays in the HDI branch and to DataArrays in the ETI branch, then read `.dims` off the second. The ETI bounds now get their own names and convert to arrays before the branches rejoin, which is what the shared tail did anyway. `pymc_models` passed a `Hashable` dimension name as a key into a `dict[str, Any]` of coords. Dimension names are strings here and PyMC requires string coord keys. `operating_characteristics` passed a list to `tight_layout(rect=...)`, which takes a 4-tuple. `diff_in_diff` iterates the "bodies" entry of a violinplot result, which matplotlib types as a single Collection rather than a list of them. That one is a stub limitation, so it takes an inline ignore, and `warn_unused_ignores` will flag it once matplotlib describes the return properly. The allowlist drops from nine entries to three, and from 49 errors in 16 files to 10 in 5. Full suite: 2284 passed, 18 skipped.
anevolbap
force-pushed
the
chore/mypy-shrink-allowlist
branch
from
August 10, 2026 11:40
911652c to
7c41e50
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stacked on #1146. Takes the allowlist from nine entries to three, and the branch from 30 errors in 13 files to 10 in 5.
Four clusters, not 20 separate bugs. In every case the annotation was narrower than the value that actually reaches it, and the runtime behaviour was already correct.
round_numdeclaredn: floatwhile four call sites pass a zero-dimensional DataArray from.mean()on posterior samples. xarray formats those like the scalar they wrap, so this was purely the annotation. Widened tofloat | xr.DataArray, matching its siblingconvert_to_string, which already declared that union. Clears 4 errors in four modules.Six constructors take
data: DataFrameLikeand rebind that same parameter to the pandas frameto_pandasreturns. mypy keeps the declared parameter type, so.index,.copy()anddata[column]were all reported againstNativeDataFrame. The conversion helpers were already annotated-> pd.DataFrame; the information was lost at the rebinding. The converted frame now gets its own name, which is also what the existing comments in those constructors already claimed. Clears 9 errors.Then five one-liners:
np.abson a DataArray replaced by the builtinabs, which xarray types correctly and which computes the same thing; xarray dimension names sorted as strings, sinceHashableis not orderable and they only go into an error message; the ETI branch of_interval_bound_valuesgiven its own names so it stops colliding with the array-typed HDI branch; a dimension name cast tostrfor a coords key that PyMC requires to be a string anyway; and a list turned into the 4-tupletight_layout(rect=...)documents.One inline ignore, on iterating the "bodies" entry of a violinplot result. matplotlib types every entry as a single Collection rather than a list of them, so this is a stub limitation rather than our bug, and
warn_unused_ignoreswill flag the ignore once matplotlib describes the return properly.What is left, and why
The remaining 10 errors are one question, not a backlog. Several attributes and parameters are declared wider than the values that reach them, and narrowing them means deciding what the model lifecycle promises:
self.modelis declared as the base model union oninverse_propensity_weightingandinstrumental_variable, while both reachPropensityScore.fitandInstrumentalVariableRegression.fit, which take numpy arrays and extra keywords (6 errors). Both call sites already carrytype: ignore[call-arg]for this, withoutarg-type. Narrowing the attribute to the subclass each experiment requires is the real fix, and it is a public type on the experiment classes.self.idatastarts asNoneand is aDataTreeat every point that returns it (2 errors), andself.causal_impactis the same shape (1 error).Axes.get_figureis Optional for a detached artist, which cannot happen at that call site (1 error).I did not want to pick an answer to the first one inside a cleanup PR, and asserting non-None at the other three would be adding runtime checks for states that cannot occur just to satisfy the checker. Happy to do whichever you prefer.
Since opening this, #1149 takes a position on all four and gets the allowlist to empty, each decision marked in the code. It is a draft, and it stacks on this branch, so this PR still stands on its own if you would rather stop here.
Verification
mypygreen under--python-version 3.12and3.14.prek run --all-filesclean, including the regeneratedenvironment.yml.Stacked on a feature branch, so this gets the repo-wide workflows but not the test matrix until the stack merges down.