refactor: restructure the api docs (4/4) - #1287
Draft
selmanozleyen wants to merge 15 commits into
Draft
selmanozleyen wants to merge 15 commits into
selmanozleyen wants to merge 15 commits into
Conversation
The module holds the public parameter bags, and later the result tuples that go with them. Those are not experimental, so `squidpy.types` is where the whole set belongs -- and introducing it under `experimental` only to move it a PR later is churn a reviewer should not have to follow. Its docs section moves with it: it was rendering as `### Types` nested under `## Experimental`, so the page filed a top-level module as an experimental one. Imported by `squidpy/__init__` rather than left to arrive as a side effect: it resolved before only because `experimental.im._detect_tissue` imports it eagerly, so `sq.types` would have vanished the moment `experimental` became lazy, while `import squidpy.types` kept working.
The ``**background_detection_params`` entry landed before ``inplace``, and ``border_margin_px`` ahead of the two parameters it follows, so the rendered parameter list disagreed with the signature above it.
``any_corner`` is an internal helper in ``_detect_tissue``: not exported, not importable, and nothing a reader of the public params class can look up. State the fallback instead of pointing at it.
``validate_qc_params``, ``validate_reinhard_params`` and ``validate_stitch_params`` are internal helpers: not exported, not importable, nothing a reader of the public params class can look up. The surrounding sentences said what every params class already says by being one, so they go with them.
Public classes in `experimental` declare their kind in the suffix: `*Fit` is an output carrying the operations that apply it, `*Params` is inert input the caller fills in. With `transform` and `decompose` on it, this is a fit by that definition -- and it was the only output class left without the marker, so the index read as if it were a third kind of thing. Deliberately not renamed before this commit: without behaviour it was arguably just a parameter bundle, and the methods are what make `*Fit` honest. `fit_stain_reference() -> StainFit` leaves the verb naming the operation and the noun naming what comes back. The function keeps its name: it fits a stain *reference*, which is what the object is; the suffix says what kind of thing it is, not what it models. No deprecation alias: the module is experimental and says so.
`eq=False` and the explicit `__eq__`/`__hash__` went out with the commit that added the
methods, which left `==` and `hash()` raising on a class that shipped in v1.8.3:
a == b ValueError: truth value of an array ... is ambiguous
hash(a) TypeError: unhashable type: 'numpy.ndarray'
a in [b] ValueError
The dataclass-generated `__eq__` compares field tuples, and comparing array fields is what
raises. `a == a` still answers True by identity short-circuit, so a smoke test passes while
`fit in cohort_fits` blows up on the first non-identical element.
The test that replaced the old one asserted the raising as intended, on the grounds that the
deleted code "silently answered by identity". It did not: it compared `method` plus
element-wise arrays, and the test it replaced asserted exactly that two distinct fits with
equal arrays compare equal. Identity was only ever the *hash*, deliberately, because array
fields cannot produce a value-based one.
Both were closed vocabularies of strings that callers write as strings, and neither earned the class it was declared as. `DetectTissueMethod` used `enum.auto()`, so its members carried no value anyone read; every use was an equality test, and a string argument was coerced through `DetectTissueMethod[method.upper()]` before any of them ran. It was never exported either, while its own docstring told callers to pass `DetectTissueMethod.OTSU` -- advice they could not follow. The `.upper()` in that coercion is why `method` is lowered before the new membership check: `"OTSU"` was accepted before and still is. `QCMetric` was a `StrEnum`, so its members already were their own strings and worked as registry keys unchanged. Its validation, though, was `isinstance(m, QCMetric)`, which is False for the plain string the member compares equal to: `qc_image(metrics="tenengrad")` raised while the docs advertised a `StrEnum`. Both now validate against `get_args`, which accepts what callers actually write. Two behaviour changes worth naming. An unknown metric now raises `ValueError` rather than `TypeError` -- it is a bad value, not a bad type, and the old class was chosen to complain about enum membership. And `QCMetric` leaves the module namespace entirely, not just `__all__`: a page rendering as nothing but `alias of Literal[...]` documents an argument rather than a type callers hold, and the names it listed now sit on the parameter that takes them, in both `im.qc_image` and its `pl` counterpart. `InputKind` stays an enum. It is internal and never crosses the public boundary.
The API page was one flat list per area, every line repeating the module it belonged to, and `experimental` was a single block interleaving `im`, `tl` and `pl`. Group it: every section names its module, `experimental` splits by submodule and then by what the entries are for, and `neighbors` moves under Graph so `GraphMatrixT` is documented once rather than beside the `gr` functions, where a bare type variable read as public API. `squidpy.types` gains the two result tuples alongside the parameter bags, and the params leave `im`/`tl`'s `__all__` so it is the single public route to them. Nine names were public but absent from the page, among them `detect_tissue`, `make_tiles` and `qc_image`. Docs machinery, so the above renders: attributes inline with their types rather than an untyped summary table, `navigation_depth` at 5 so a section unfolds to its pages instead of stopping at the sub-section, and page titles as the bare name rather than the dotted path repeated in every nav entry. `typeddict.rst` goes: it was byte-identical to the built-in `base.rst` it shadowed, so it rendered nothing the default did not.
The Python domain renders a typed field inline as ``name (type) - description`` inside a two-column grid, so the three things a reader scans for share one run-on line indented behind the "Parameters:" label. A doctree transform splits each entry into ``name : type`` and its prose, and the field list is laid out as blocks rather than a grid. ``typehints_defaults`` puts each default next to its type. The signature line gets the name at a size worth landing on, with the module path receding behind it.
``pl.qc_image`` respelled every type the annotation already gives and named its return twice; ``tl.make_stitched_labels`` and ``pl.tiling_qc`` documented no return at all. Each parameter now renders its own ``(default: x)``, so the inline ``(default)`` markers duplicate it -- the computed ones, which no signature can show, stay. ``QCMetric`` is a fifteen-value alias that ``qc_image`` spelled out twice; it renders by name.
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
1 participant
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.
4th step of #1279