refactor(carbon): move NSVB scalar test-oracle out of src into tests (#127.1) - #129
Open
mihiarc wants to merge 1 commit into
Open
refactor(carbon): move NSVB scalar test-oracle out of src into tests (#127.1)#129mihiarc wants to merge 1 commit into
mihiarc wants to merge 1 commit into
Conversation
…127.1) The production NSVB path is fully vectorized (polars). A parallel *scalar* reimplementation of the same math shipped in src/ purely as a test oracle (equations.py Models 1-5 / predict_tree_biomass / harmonize_components / _eval_component / Coefficients / TreeBiomassResult; coefficients.py lookup_coefficients / _row_to_dict / ecosubcd_to_division). Move it to tests/nsvb_oracle.py so the shipped library no longer carries a second implementation, wired via `pythonpath = ["tests"]`. The oracle is kept (not deleted): test_nsvb_vectorized.py cross-checks the vectorized pipeline tree-for-tree against this independent implementation, and both are anchored to the hand-computed GTR-WO-104 worked-example values in test_nsvb_equations.py — a cross-check that has caught real bugs (the #123 half-implemented Model 2). Removing it would trade away that independent oracle. Scope note: the carbon_fractions.py dict loaders were left in src. They looked test-only but are production dependencies — `_compute_default_{live,dead}_carbon_fraction` (used by live_tree/standing_dead) derive their means from them — so only the equations + coefficients scalar reference moved. Verified before removing, per the "check the coupling first" guidance. No production code path changed: shared constants (`_K_*`, `_CULL_DENS_PROP`), `load_nsvb_coefficients`/`CoefficientTables`, the `build_*`/`get_vectorized_lookup_tables` loaders, `ecosubcd_to_division_expr`, and the `_df` fraction loaders all stay. Carbon point estimates are byte-identical on GA/NV (ag+dead). 924 unit + 12 property/validation tests pass; ruff + mypy clean.
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.
Resolves the deferred item from #127: the parallel scalar NSVB reimplementation that shipped in
src/purely as a test oracle.What moved
src/pyfia/carbon/nsvb/{equations,coefficients}.pyscalar reference code →tests/nsvb_oracle.py, wired viapythonpath = ["tests"]:equations.py:model_1–model_5_jenkins,_model_k,harmonize_components,_eval_component,predict_tree_biomass,Coefficients,TreeBiomassResultcoefficients.py:lookup_coefficients,_row_to_dict,ecosubcd_to_division(the scalar one;ecosubcd_to_division_expris production and stays)The shipped library is now ~640 lines lighter and no longer carries a second implementation of the biomass math.
Why keep the oracle (not delete it)
test_nsvb_vectorized.pycross-checks the production vectorized pipeline tree-for-tree against this deliberately-independent implementation, and both are independently anchored to the hand-computed GTR-WO-104 worked-example values intest_nsvb_equations.py. That independent cross-check has already caught a real bug (the #123 half-implemented Model 2). Verified up front that the vectorized path is independently ground-truthed (the*_worked_example_exacttests assertcompute_nsvb_biomassdirectly against the GTR sentinels), so the oracle is a genuine second check, not the sole source of truth.Scope note — a coupling caught before removing
The issue also listed the
carbon_fractions.pydict loaders. They stayed in src: they turned out to be production dependencies —_compute_default_{live,dead}_carbon_fraction(used bylive_tree/standing_dead) derive their means fromload_carbon_fractions_{live,dead}(). Only the two thin test-only getters would have been movable, which isn't worth splitting the module for. So only the equations + coefficients scalar reference moved.Safety
No production code path changed. Shared constants (
_K_*,_CULL_DENS_PROP),load_nsvb_coefficients/CoefficientTables, thebuild_*/get_vectorized_lookup_tablesloaders,ecosubcd_to_division_expr, and the_dffraction loaders all stay in src. Carbon point estimates are byte-identical on GA/NV (ag + standing-dead). 6 test modules repointed toimport nsvb_oracle.924 unit + 12 property/validation tests pass; ruff + mypy clean.