Skip to content

Fix docstrings that name parameters the functions do not have - #3485

Open
VenishPaneliya wants to merge 1 commit into
pyro-ppl:devfrom
VenishPaneliya:docstring-param-names
Open

VenishPaneliya wants to merge 1 commit into
pyro-ppl:devfrom
VenishPaneliya:docstring-param-names

Conversation

@VenishPaneliya

Copy link
Copy Markdown

Nine Sphinx field entries name an argument the function does not take. Sphinx renders each as a parameter that does not exist, while the real argument gets no entry at all.

file function documented actual
ops/jit.py trace ignore_warnins ignore_warnings
ops/provenance.py get_provenance tensor x
ops/provenance.py detach_provenance tensor x
infer/mcmc/util.py initialize_model ignore_jit_warnings skip_jit_warnings
infer/reparam/reparam.py Reparam.apply name msg
distributions/coalescent.py __call__ time t
contrib/funsor/.../enum_messenger.py queue q queue
contrib/epidemiology/compartmental.py compute_flows state prev, curr
contrib/epidemiology/compartmental.py generate :pram :param

Several of these are self-evident from their surroundings:

  • ignore_warnins is simply missing a letter.
  • :pram dict fixed: is the only :pram in the package — every other field in pyro/ is a correct :param.
  • get_provenance / detach_provenance sit directly below extract_provenance, which has the same (x) signature and already documents :param x: correctly. I reused its exact wording for get_provenance, since that function's own summary says it reads "a recursive datastructure possibly containing torch.Tensor s" rather than a tensor; detach_provenance keeps the torch.Tensor type, matching its x: _Tensor annotation.
  • Reparam.apply documents name, but the entry itself describes "A simplified Pyro message with fields…", i.e. msg.
  • CoalescentRateLikelihood.__call__ documents time while the :type t: int or slice line just below already refers to t.
  • initialize_model documents ignore_jit_warnings; pyro.util.ignore_jit_warnings is a separate context manager, which is likely where the name came from.

compute_flows — the one that needed more than a rename

compute_flows(self, prev, curr, t) carried this entry:

:param dict state: A dictionary mapping compartment name to current
    tensor value. This should be updated in-place.

That text is copied verbatim from transition(self, params, state, t) earlier in the same file, where state is a real argument and is updated in place. compute_flows takes two dicts and returns a new one — neither prev nor curr is mutated, in the base implementation or in either override in models.py. So the entry is replaced with one per argument and the in-place note dropped. transition's own entry is left untouched.

Deliberately not touched

  • contrib/mue/models.py and contrib/mue/missingdatahmm.py have the same class of mismatch, but nothing in the package imports contrib.mue.models, so I left that area alone rather than patching docs there speculatively.
  • distribution.sample, importance.py, and ops/einsum look like hits to a naive scan, but they document genuine **kwargs keys (e.g. cache_path is read via kwargs.pop("cache_path", True)), which is correct as written.

Docstrings only, no behaviour change. ruff check and ruff format --check pass on all seven files, and every name above was confirmed against inspect.signature on the built package.

Nine Sphinx field entries name an argument that is not in the signature,
so Sphinx renders a parameter that does not exist while the real one is
undocumented:

- `pyro.ops.jit.trace`: `ignore_warnins` is a typo for `ignore_warnings`.
- `get_provenance` / `detach_provenance`: both document `tensor` for a
  parameter named `x`. `extract_provenance` directly above them, with the
  same signature, already documents `x` correctly, so its wording is
  reused for `get_provenance`, whose summary describes a data structure
  rather than a tensor.
- `initialize_model`: documents `ignore_jit_warnings`; the argument is
  `skip_jit_warnings`. `pyro.util.ignore_jit_warnings` is a separate
  context manager, which is probably where the name came from.
- `Reparam.apply`: documents `name`; the argument is `msg`, and the
  entry already describes a message rather than a name.
- `CoalescentRateLikelihood.__call__`: documents `time`; the argument is
  `t`, which the `:type t:` line below already refers to.
- `contrib.funsor` `queue`: documents `q`; the argument is `queue`.
- `CompartmentalModel.compute_flows`: documents `state`, which is copied
  verbatim from `transition` above it, where `state` is a real argument
  and is updated in place. `compute_flows` instead takes `prev` and
  `curr` and returns a new dict, so the entry is replaced by one for each
  and the in-place note is dropped.
- `CompartmentalModel.generate`: `:pram` is a typo for `:param`, the only
  one in the package.

Docstrings only, no behaviour change.

This branch has not been deployed

No deployments
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.

1 participant