Repository navigation
The basis= dict forwards every key, and three concepts lose their second names - #90
Merged
Merged
Conversation
Fisher and PICSLike each filtered the dict through a hand-kept tuple that
listed five keys. Anything else was dropped before the call, so
basis={"method": "harmonic", "compress": True} ran uncompressed while the
log reported the harmonic path, and m-block compression was unreachable
except by constructing HarmonicBasis directly.
One Core._basis_setup_kwargs() replaces both tuples and validates against
Core.setup_computation_basis's own signature, so there is no list left to
drift and an unrecognised key raises instead of vanishing. Read off Core,
not self: PICSLike wraps the method as (*args, **kwargs), which carries no
parameter names. Spectra needed nothing, it forwards the resolved dict to
an internal Fisher.
Fisher also released the pixel projector unconditionally, which the
harmonic basis refuses when compressed because the m-block path keeps
reading V. Guarded, and the path label now names delta_m.
The constructor named it compression_target; five methods on the same
class called it basis, and plot_eigenvalue_comparison called it bases.
That word is already taken: basis= on Fisher, Spectra and PICSLike
selects harmonic vs pixel vs auto, so basis="noise_weighted" and
basis={"method": "harmonic"} were two concepts sharing one spelling in
one package. Residue from the ADR-0002 vocabulary work, which renamed
the constructor and missed these.
The four private methods are renamed outright. The four public ones take
compression_target= (compression_targets= on the comparison plot) and
keep the old spelling for one release through a shared warn-and-forward
helper, per ADR-0018. Passing both raises.
Also documented, not renamed: params_file= on the three orchestration
classes has always accepted an InputParams or a dict, not just a path,
and the annotation said otherwise. And PixelBasis's class docstring
documented a basis constructor parameter that never existed.
Two more places the codebase carried two names for one concept. Fisher, Spectra and PICSLike took params_file=, while Core has always called the same argument params and has always accepted an InputParams, a path or a dict. "file" named one of the three, and the annotation claimed only that one. The parameter is now params=, resolved through a shared Core._resolve_params_alias; params_file= warns and forwards for one release, and passing both raises. Almost every caller passes it positionally and is untouched. PixelBasis.available_bases() and cosmocore.basis.COMPRESSION_BASES were the last two "basis" spellings of the compression target. They are now available_compression_targets() and COMPRESSION_TARGETS; the constant keeps its old spelling through a module __getattr__ so the deprecated read still warns rather than silently aliasing. Docs and examples across the READMEs, index and picslike API pages use the new spellings.
cross_idxs and auto_idxs are numpy arrays on every instance, so the all(a == b for ...) generator called bool() on an array and raised "truth value of an array with more than one element is ambiguous". InputParams() == InputParams() was enough to trigger it, which makes the docstring's claim that this is useful for validating configurations false for every real config. A _values_equal helper routes the array case through np.array_equal and leaves everything else on ==. Only the top level needs it: no config in the test data holds a list or dict of arrays. The params_file alias test in picslike goes back to comparing the two InputParams directly, which is what it wanted to do.
Core._resolve_params_alias and pixel.py's _resolve_target_alias were the same function with different message strings. They are now one resolve_alias in a leaf _deprecation module: core imports basis, so anything both need cannot live in core, and basics is numerical primitives. The three singular call sites in pixel.py were repeating a six-line resolve-then-default block with the same two name literals every time. _resolve_target bakes in the names and the default, so each is one line. Also, two of the four "C_ell required for ..." messages had picked up compression_target as a noun from the rename sweep while their siblings said target. No behaviour change. Every deprecation warning still reports the user's own line, verified for all five aliases.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #90 +/- ##
==========================================
- Coverage 92.66% 92.57% -0.10%
==========================================
Files 40 41 +1
Lines 5088 5130 +42
==========================================
+ Hits 4715 4749 +34
- Misses 373 381 +8
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
🟡 Changes recommended
A small but user-facing bug in PixelBasis target resolution can silently accept invalid falsy inputs (e.g. empty string) instead of raising the expected “unknown target” error.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR fixes a silent basis-configuration forwarding bug (making harmonic m-block compression reachable via basis={...}), standardizes several public API names with one-release deprecation shims (ADR-0018), and corrects InputParams.__eq__ to handle array-valued attributes.
Changes:
- Forward
basis={...}dict keys based onCore.setup_computation_basis’s signature and raise on unknown keys (enablingcompress/delta_mand preventing silent drops). - Deprecate/alias
params_file=→params=across orchestration classes and rename PixelBasis “basis” terminology to “compression_target(s)” with compatibility shims. - Fix
InputParams.__eq__to compare ndarray attributes safely and update docs/tests accordingly.
File summaries
| File | Description |
|---|---|
| src/cosmoforge.qube/tests/test_basis_kwarg.py | Adds regression tests for compress forwarding, unknown-key validation, and params_file= deprecation. |
| src/cosmoforge.qube/README.md | Updates example to use params=. |
| src/cosmoforge.qube/qube/spectra.py | Introduces params= (with params_file= deprecated alias) and updates docstrings. |
| src/cosmoforge.qube/qube/fisher.py | Uses signature-driven basis kwargs forwarding, improves basis path labeling for m-block, and avoids releasing V when compressed. |
| src/cosmoforge.picslike/tests/test_picslike.py | Updates call sites to params= and adds explicit tests for the params_file= alias behavior. |
| src/cosmoforge.picslike/picslike/picslike.py | Introduces params= (with params_file= deprecated alias) and uses signature-driven basis kwargs forwarding. |
| src/cosmoforge.picslike/docs/README.md | Updates example to use params=. |
| src/cosmoforge.meta/README.md | Updates example to use params=. |
| src/cosmoforge.cosmocore/tests/test_settings.py | Adds coverage for InputParams.__eq__ behavior with ndarray/scalar differences. |
| src/cosmoforge.cosmocore/tests/test_pixel_basis.py | Updates tests to new “compression_target(s)” naming and adds alias tests. |
| src/cosmoforge.cosmocore/README.md | Updates PixelBasis examples to compression_target= spelling. |
| src/cosmoforge.cosmocore/cosmocore/settings.py | Implements ndarray-safe equality via _values_equal and updates __eq__. |
| src/cosmoforge.cosmocore/cosmocore/core.py | Adds params_file alias resolver and signature-driven basis kwarg validation/forwarding. |
| src/cosmoforge.cosmocore/cosmocore/basis/pixel.py | Renames compression “basis” concept to “compression_target(s)” and adds deprecation shims. |
| src/cosmoforge.cosmocore/cosmocore/basis/init.py | Renames exported registry to COMPRESSION_TARGETS and provides a deprecated COMPRESSION_BASES module attribute. |
| src/cosmoforge.cosmocore/cosmocore/_deprecation.py | Adds shared resolve_alias helper for one-release warn-and-forward aliases. |
| README.md | Updates example to use params=. |
| docs/source/index.rst | Updates quick-start snippets to use params=. |
| docs/source/changelog.rst | Adds Unreleased notes covering the forwarding fix and deprecations. |
| docs/source/api/picslike/picslike.rst | Updates docs examples to use params=. |
| docs/source/api/picslike.rst | Updates docs examples to use params=. |
| CONTEXT.md | Updates m-block compression note to reflect the public basis={...} spelling. |
Review details
- Files reviewed: 22/22 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
_resolve_target fell back to the default on any falsy value, so compression_target="" was quietly replaced by "noise_weighted" instead of reaching the validation that rejects it. That is a regression: before the rename, basis="" raised "Unknown compression basis ''". Copilot review on the pull request.
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.
What changed
A reachability bug and the naming debt sitting next to it.
basis={"compress": True}was silently dropped and ran uncompressed, because
FisherandPICSLikeeachfiltered the dict through a hand-kept tuple of five keys; the forwarded set is now
the signature of
setup_computation_basisitself, and an unrecognised key raises.Fixing that seam exposed two places carrying two names for one concept, so those are
renamed here too, each with a one-release warn-and-forward alias per ADR-0018. One
unrelated crash found on the way:
InputParams.__eq__raised on any real config.Public surface
Fixed
basis={"compress": True, "delta_m": N}reaches the basis. m-block compression waspreviously unreachable except by constructing
HarmonicBasisdirectly, so the testspassed while no user could turn the feature on.
beamanduse_smw_optimizationare now accepted in the dict too, since the forwarded set follows the signature.
basis=dict raisesValueErrorinstead of vanishing.InputParams.__eq__compares configurations instead of raising.cross_idxsandauto_idxsare arrays on every instance, so evenInputParams() == InputParams()raised
ValueError: The truth value of an array ... is ambiguous.PixelBasis's class docstring documented abasisconstructor parameter that hasnever existed; the constructor takes
compression_target.Deprecated (old spelling works for one release and warns)
PixelBasis.compute_eigenspectrum,compute_eigenspectrum_per_fieldandplot_eigenvalue_spectrumtakecompression_target=;plot_eigenvalue_comparisontakes
compression_targets=. Theirbasis=/bases=collided withbasis=onFisher,SpectraandPICSLike, which selects harmonic vs pixel vs auto.params_file=onFisher,SpectraandPICSLikeis nowparams=, the nameCorehas always used. It has always accepted anInputParams, a path or a dict.Callers passing it positionally, which is nearly all of them, are unaffected.
PixelBasis.available_bases()is nowavailable_compression_targets(), andcosmocore.basis.COMPRESSION_BASESis nowCOMPRESSION_TARGETS.Notes for review
resolve_aliasin the new leafcosmocore/_deprecation.py. It cannot live incore(coreimportsbasis) andbasicsis numerical primitives. Passing both spellings raisesTypeError.caller's own line rather than a line inside the library.
rtol=0.1on the Fisher diagonal.delta_m=0assumes uniform per-ring phi sampling, which HEALPix at nside=4 is farfrom, so the diagonal drifts from ~1e-4 at low ell to ~8% at the top multipole. The
sharp accuracy claim stays in cosmocore's
test_symmetric_mask_fisher_accuracy, ona geometry that earns it.
Fisherno longer callsrelease_pixel_projector()unconditionally: the harmonicbasis raises on that when compressed, because the m-block path keeps reading V.
Checklist
docs/source/changelog.rstupdated under UnreleasedCONTEXT.mdupdated — the m-block line now names the public spellingindex.rstand the picslike API pages updated;sphinx builds with 36 warnings, identical to the pre-change build
1 deselected, picslike 106 passed / 2 skipped