Skip to content

Use native NNS distance by default package-wide#127

Merged
OVVO-Financial merged 5 commits into
mainfrom
codex/fix-pr-#126-to-use-native-nns-distance
Jul 16, 2026
Merged

Use native NNS distance by default package-wide#127
OVVO-Financial merged 5 commits into
mainfrom
codex/fix-pr-#126-to-use-native-nns-distance

Conversation

@OVVO-Financial

Copy link
Copy Markdown
Owner

Motivation

  • Align the Python package with the native blended NNS default so that any public API or internal delegation that omits dist uses the native NNS formula (sum(abs(z) + z**2)) rather than implicitly retaining historical L2 behavior.
  • Ensure parity helpers and R bridge code represent None/NULL explicitly so cache keys and R calls distinguish the native default from intentionally-requested L2.
  • Add invariant tests to prevent accidental reintroduction of hard-coded dist="L2" delegations in ordinary package source.

Description

  • Switched internal delegations that previously forced dist="L2" to use dist=None (or omit the dist kwarg) in derivative and VAR call paths, primarily in src/nns/diff.py and src/nns/var.py.
  • Updated R parity helpers in tests/_r.py to accept dist: str | None = None, include "dist": dist in the helper argument dictionaries, decode args$dist to dist_arg (NULL vs character) inside embedded R scripts, and pass dist = dist_arg by name into NNS.reg, NNS.stack, and NNS.boost calls.
  • Rewrote general parity tests under tests/parity/ so default parity comparisons use Python dist=None and R dist = NULL instead of implicit L2; preserved and kept dedicated explicit-L2 tests unchanged where they assert explicit L2 behavior.
  • Added focused invariant tests in tests/invariants/test_native_distance_defaults.py that (1) assert derivative helpers delegate with native None (monkeypatched checks) and (2) scan package source AST to fail if an ordinary package call delegates with a hard-coded dist='L2'.
  • Kept validator/implementation locations and dedicated explicit-L2 tests unchanged; docstrings and R manual pages that describe explicit dist="L2" remain as documentation.

Testing

  • ruff check . completed successfully.
  • python -m compileall -q src tests scripts completed successfully (AST/compile sanity OK).
  • Ran unit/test entrypoints; many pytest runs could not execute here because required runtime dependencies (notably numpy) and R (Rscript) are not available in this environment, so live R parity and full pytest runs are blocked.
  • python scripts/regenerate_r_cache.py --fresh -- tests/parity could not run because Rscript is not on PATH in this environment and so the R cache could not be regenerated here; the PR includes the helper changes required prior to regeneration but the actual cache rebuild must be performed in the authorized CI/regeneration environment and committed afterwards.
  • mypy and python -m build were attempted but were blocked or produced missing-import diagnostics due to the same unavailable build/dependency tooling; these checks should be re-run in CI where dependencies are installed.

Notes on remaining hard-coded dist="L2" occurrences: the only allowed remaining matches are explicit L2 tests and documentation (for example tests/invariants/test_native_distance_defaults.py explicit-L2 assertions and R manual pages); there are no remaining hard-coded dist="L2" delegations inside ordinary package source after this change, and the new invariant test will detect regressions.

If you want, I can (1) run the official R cache regeneration in the proper environment and update tests/_r_cache.json, or (2) open follow-up CI job output to validate live R parity and cache-only parity once the pipeline runs in CI with numpy and R installed.


Codex Task

@OVVO-Financial
OVVO-Financial merged commit af34b08 into main Jul 16, 2026
18 checks passed
@OVVO-Financial
OVVO-Financial deleted the codex/fix-pr-#126-to-use-native-nns-distance branch July 16, 2026 21:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant