refactor: convert experimental parameters to TypedDicts (1/4) - #1279
selmanozleyen wants to merge 37 commits into
Conversation
cd1fa5b to
36451d4
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1279 +/- ##
==========================================
+ Coverage 78.44% 78.90% +0.46%
==========================================
Files 63 64 +1
Lines 9532 9496 -36
Branches 1594 1566 -28
==========================================
+ Hits 7477 7493 +16
+ Misses 1489 1459 -30
+ Partials 566 544 -22
🚀 New features to boost your workflow:
|
d1001aa to
affe9e0
Compare
Brings in scverse#1279 (params as TypedDicts with their defaults declared on the key), scverse#1280 (StainFit and its methods), the enum-to-Literal pass, and the API page restructuring, so the align work sits on the shape those settle rather than carrying its own copy of it. Conflicts resolved toward the stack for everything it owns: the params, the defaults machinery, the enum conversion and the docs templates all come from there, and the three copies this branch had made of them are dropped. The alignment surface is what this branch adds on top -- the `stalign_align_*` entry points, the fit classes they return, and `rasterize_points`/`sample_volume`. `Stalign*Params` move into `squidpy.types` with the rest. Re-exporting them from `_align._stalign` deadlocked at import: `types` would have imported the implementation package whose `__init__` imports `types`. They are declared there now, like every other params class, and `_stalign` reads them back. `_matches` in the params test grew a tolerant union arm. It walks a key's declared type to check the default against it, and `npt.ArrayLike` unions in protocols that are not `runtime_checkable`, so `isinstance` raised before reaching the `None` arm that actually matched.
5ef501c to
5146abb
Compare
5146abb to
0586b10
Compare
|
Nice, I like the plan and consistency. What I don’t get is the distinction between spelling kwargs out in the function and having them defined in a TypedDict: are they always shared? Is there a rule about what goes where? |
|
|
hi @timtreis, so these are what I noticed on the kwargs bags. I think it's useful to rethink them:
|
selmanozleyen
changed the title
6dcb8f6 to
c522628
Compare
|
Very nice design, I think this 3-way split makes the most sense.
I think there should be an indicator that the 
They invent custom object types, which isn’t ideal for interoperability (i.e. if you use it, it’ll be hard for users to link to them), but your call if that’s worth it. |
|
sphinx_toolbox.more_autodoc caused some compat issues because it upper bounded a version of an extension which we use the latest version of. I just added a one line mentioning they are a type dict. |
ae5d9d7 to
694c55e
Compare
Replaces the autodoc hook that injected the note at build time: plain docstrings also show up in help() and IDEs, and cannot silently drop out.
StitchParams was six keys unpacked into assign_stitch_groups, its only
consumer: the same case as TilingQCParams, so the keys are plain keyword
arguments now and are recorded flat in .uns['tiling_stitch'].
The hint calculate_tiling_qc logs when a re-run drops the stitch columns
printed stitch_params={...}, which the **kwargs signature rejects. It now
prints the flat keys, and a test runs the printed call.
When a setting stays a keyword argument, when it becomes a *Params bag (method_params= vs **Unpack), and what Fits and Results are.
Every *_DEFAULTS constant in squidpy.types only existed to be passed to resolve_params. It now takes the TypedDict itself and reads its defaults through a cached defaults_of, so the constants are gone. The import-time 'every key has a Default' check they gave is covered by test_params.
- corners_are_background accepts a numpy array of four bools (it raised), and refuses a string instead of reading "False" as True - n_neighbors < 1 and a fractional min_area raise again, as on main: min_area is coerced before its check, not after - the new tuning knobs of calculate_tiling_qc and assign_stitch_groups are keyword-only and last, so existing positional calls keep their slots - the stitch re-run hint names qc_table_key when the QC table is not the default one
…nature The private helpers repeated them as their own defaults, but assign_stitch_groups always passes every value, so the helpers take them as required arguments now and the signature is the one place they are declared.
squidpy.types imported them from squidpy.experimental, the wrong way round for a module that is not experimental. A @validates(Spec) decorator now registers each validator with its *Params class, so resolve_params(p, Spec) always runs it, and the per-method _resolve_*_params wrappers are gone. The method_params docstrings no longer restate the types the annotation already names.
DEFAULT_LUMINOSITY_THRESHOLD was used once, as that key's Default, and only re-exported otherwise; its explanation moves into the key's docstring.
MacenkoParams and VahadaneParams take the same mean-absorbance cutoff, so it is declared once, on _ODBetaParams, and the _OD_BETA constant goes.
A TypedDict does not keep its bases in its MRO, so autodoc cannot document keys inherited from one: the shared base hid beta from both classes' docs. Each class declares it with Default(0.15), and no constant is needed.
…ate WEKA border margin, strict corner flags
…te once per module
e523269 to
75b4beb
Compare
06b7fa8 to
0c3243f
Compare
This branch has not been deployed
2 participants
Hi,
I want to bring a clear separation to the kwargs madness we have here.
StainReferenceorAlignmentResultwe use frozen data classes . Even if they are given as input later to other functions since we don't expect the user to type those it doesn't make sense to unpack. Based on this instinct I also have a follow up PR to stack on refactor: add StainReference transform methods (2/4) #1280NamedTuple(like current SpatialNeighboursResults)(about the nesting of functions by groups I am open to discussion, but it's really unreadable if we have a flat
tl.*. I would also argue that we want to deprecate.imanyway. So we might as well put everything insideexperimental.im.*undertlto make it less confusing if we will do this grouping in the docs.Intended end result after one more pr in this stack:
Fits
Functions
Results
Params