Repository navigation
docs: write the public comments for a caller, not an implementer - #38
Merged
Merged
Conversation
tnagler
force-pushed
the
docs/public-api-comments
branch
from
August 26, 2026 12:25
2c27845 to
abbbe4d
Compare
Contributor
|
Rgarding get_discrete_support_min / get_discrete_support_max becoming public: good idea. |
tvatter
force-pushed
the
docs/public-api-comments
branch
from
August 27, 2026 07:12
abbbe4d to
aa2fbce
Compare
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
force-pushed
the
docs/public-api-comments
branch
from
August 27, 2026 07:21
aa2fbce to
204552d
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
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 #37 (
discrete-boundary-transforms).pyvinecopulib's docstring generator lifts these
//!comments verbatim intoits 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_sizeblamed the difference on "boundary-snapping infinalize_grid()".finalize_gridonly reverses now, and the count isalways exactly one more than requested, never "slightly" different. It also
cross-referenced a private method.
quantiledocumented 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 saidthe same wrong thing. Both now say
[0, 1].get_edfdescribed "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_enumaccepts"zero_inflated"as well as"zero-inflated", butneither
@param typenor the error message listed it.@param prob0while declaring theparameter
prob0_, so a doc tool sees one undocumented parameter and one@paramfor a parameter that does not exist. The trailing underscore was thebug; the declarations now match the definitions.
Under-documented, in ways that matter downstream
xmin/xmaxmean now differs by variable type — integer supportfor 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_valuesdid not say that a discrete grid runshalf a unit past each bound, nor that its values are the jitter density
rather than probabilities.
fitlisted 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_xmaxdid not mention that it throws after a fit, or on anon-integer discrete bound.
str()had no comment at all, so the Python__repr__inherits an emptydocstring.
VarTypeand its three members had none either.Rewritten away from mechanism
The class summary,
get_bandwidth,get_boundary_repairand@param boundary_repairled 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_repairalso states its default and that it is eligibilityrather than a guarantee.
Smaller
@param check_fittedsaid "logical", which is R's word, and read backwards(passing
trueperforms the check).@param seedsnever said what an emptyvector does.
@param bandwidth's continuation line was unindented, whichtruncates the description in a converter that uses indentation to find one.
@param typecarried a double space that survives into rendered text.Not in this PR, but worth a decision
get_discrete_support_min/get_discrete_support_maxare private. They arethe 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.
diagnostic/— a 170 KB PNG,PR_COMMENT.mdand 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.