Skip to content

docs: write the public comments for a caller, not an implementer - #38

Merged
tvatter merged 1 commit into
mainfrom
docs/public-api-comments
Aug 27, 2026
Merged

tvatter merged 1 commit into
mainfrom
docs/public-api-comments

Conversation

@tvatter

@tvatter tvatter commented Aug 26, 2026

Copy link
Copy Markdown
Contributor

Stacked on #37 (discrete-boundary-transforms).

pyvinecopulib's docstring generator lifts these //! comments verbatim into
its Python API documentation, converting Doxygen tags to numpydoc. Whatever is
written here is what a Python user reads on the rendered site — so an
implementation note here becomes user-facing API text there. Bumping the
submodule to #37 is what surfaced these; none of them are new to #36/#37 alone.

No behavior changes, with two deliberate exceptions, both corrections to what
the code says about itself.

Factually wrong

  • get_actual_grid_size blamed the difference on "boundary-snapping in
    finalize_grid()". finalize_grid only reverses now, and the count is
    always exactly one more than requested, never "slightly" different. It also
    cross-referenced a private method.
  • quantile documented its domain as (0, 1) while the guard accepts
    [0, 1] — and Handle bounds for discrete and zero-inflated densities #37's own tests rely on the closed interval
    (CHECK(bounded.quantile(endpoints)(0) == 0.0)). The exception message said
    the same wrong thing. Both now say [0, 1].
  • get_edf described "the sum of the per-observation influence values",
    which stopped being the whole story once boundary repair fuses them, and
    never mentioned the zero-inflated +1.
  • as_enum accepts "zero_inflated" as well as "zero-inflated", but
    neither @param type nor the error message listed it.
  • Both grid constructors document @param prob0 while declaring the
    parameter prob0_, so a doc tool sees one undocumented parameter and one
    @param for a parameter that does not exist. The trailing underscore was the
    bug; the declarations now match the definitions.

Under-documented, in ways that matter downstream

  • What xmin/xmax mean now differs by variable type — integer support
    for a discrete variable, continuous-component-only for a zero-inflated one.
    After Handle bounds for discrete and zero-inflated densities #37 this is the single most important semantic in the class, and it was
    stated only in the overview pages. It is now on the constructors and on
    get_xmin/get_xmax, where a caller looks.
  • get_grid_points / get_values did not say that a discrete grid runs
    half a unit past each bound, nor that its values are the jitter density
    rather than probabilities.
  • fit listed no exceptions, though it can now raise five distinct ones,
    and did not say that NaNs are dropped or that weights are rescaled to mean
    one.
  • set_xmin_xmax did not mention that it throws after a fit, or on a
    non-integer discrete bound.
  • str() had no comment at all, so the Python __repr__ inherits an empty
    docstring.
  • VarType and its three members had none either.

Rewritten away from mechanism

The class summary, get_bandwidth, get_boundary_repair and
@param boundary_repair led with "bulk estimator", "boundary experts",
"endpoint classifier" and "fusion weights" — terms defined nowhere a Python
reader can reach. They now say what the thing is and what it means for the
caller, and point at the overview pages for the estimator itself.
@param boundary_repair also states its default and that it is eligibility
rather than a guarantee.

Smaller

@param check_fitted said "logical", which is R's word, and read backwards
(passing true performs the check). @param seeds never said what an empty
vector does. @param bandwidth's continuation line was unindented, which
truncates the description in a converter that uses indentation to find one.
@param type carried a double space that survives into rendered text.

Not in this PR, but worth a decision

  • get_discrete_support_min / get_discrete_support_max are private. They are
    the only way to ask where the integer support ends without re-deriving the
    rule, and a downstream port has to re-derive it today. Happy to make them
    public here if you want them.
  • Stabilize density estimates near support boundaries #36 added diagnostic/ — a 170 KB PNG, PR_COMMENT.md and five R scripts,
    216 KB in all. That lands in every downstream sdist that vendors this
    submodule. Worth considering whether the validation artifacts belong in the
    repository or in the pull-request thread.

@tnagler

tnagler commented Aug 26, 2026

Copy link
Copy Markdown
Contributor

Rgarding get_discrete_support_min / get_discrete_support_max becoming public: good idea.

@tvatter
tvatter force-pushed the docs/public-api-comments branch from abbbe4d to aa2fbce Compare August 27, 2026 07:12
Base automatically changed from discrete-boundary-transforms to main August 27, 2026 07:21
These `//!` comments are lifted verbatim into pyvinecopulib's Python API
documentation, so whatever is written here is what a Python user reads on the
rendered site. Several of them had drifted into describing the implementation,
and a few had become wrong.

Factually wrong:

- `get_actual_grid_size` blamed the difference on "boundary-snapping in
  `finalize_grid()`". `finalize_grid` only reverses now, and the count is
  always exactly one more than requested, never "slightly" different. It also
  cross-referenced a private method.
- `quantile` documented its domain as `(0, 1)` while the guard accepts `[0, 1]`
  and the tests added in #37 rely on the closed interval. The exception message
  said the same wrong thing; both now say `[0, 1]`.
- `get_edf` described the sum of per-observation influences, which stopped
  being the whole story once boundary repair could fuse them, and never
  mentioned the zero-inflated `+1`.
- `as_enum` accepts `"zero_inflated"` as well as `"zero-inflated"`, but neither
  the `@param type` text nor the error message listed it.
- Both grid constructors document `@param prob0` while declaring the parameter
  as `prob0_`, so a doc tool sees one undocumented parameter and one `@param`
  for a parameter that does not exist. The trailing underscore was the bug;
  the declarations now match the definitions.

Under-documented, in ways that matter downstream:

- What `xmin` / `xmax` *mean* now differs by variable type -- integer support
  for a discrete variable, continuous-component-only for a zero-inflated one --
  and that was stated only in the overview pages. It is on the constructors and
  on `get_xmin` / `get_xmax` now, since that is where a caller looks.
- `get_grid_points` and `get_values` did not say that a discrete grid runs half
  a unit past each bound, nor that its values are the jitter density rather
  than probabilities.
- `fit` listed no exceptions at all, though it can now raise five distinct
  ones, and did not say that NaNs are dropped or that weights are rescaled.
- `set_xmin_xmax` did not mention that it throws after a fit, or on a
  non-integer discrete bound.
- `str()` had no comment, so the Python `__repr__` inherited an empty one.
- `VarType` and its three members had none either.

Rewritten away from mechanism: the class summary, `get_bandwidth`,
`get_boundary_repair` and `@param boundary_repair` led with "bulk estimator",
"boundary experts", "endpoint classifier" and "fusion weights" -- terms defined
nowhere a Python reader can reach. They now say what the thing is and what it
means for the caller, and point at the overview pages for the estimator itself.
`@param boundary_repair` also states its default and that it is eligibility
rather than a guarantee.

Smaller: `@param check_fitted` said "logical", which is R's word, and read
backwards; `@param seeds` never said what an empty vector does; `@param
bandwidth`'s continuation line was unindented, which truncates the description
in a numpydoc converter that uses indentation to find one; and `@param type`
carried a double space that survives into the rendered text.

No behavior changes, with two deliberate exceptions that are corrections to
what the code says about itself: the quantile exception message, and the
`as_enum` message.
@tvatter
tvatter force-pushed the docs/public-api-comments branch from aa2fbce to 204552d Compare August 27, 2026 07:21
@tvatter
tvatter merged commit f0a6875 into main Aug 27, 2026
6 checks passed
@tvatter
tvatter deleted the docs/public-api-comments branch August 27, 2026 07:22
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.

2 participants