From 7c4dac4586adb7b3fd9047c8e8b58ba6d664e4e3 Mon Sep 17 00:00:00 2001 From: hyperpolymath <6759885+hyperpolymath@users.noreply.github.com> Date: Fri, 25 Sep 2026 21:59:58 +0000 Subject: [PATCH] fix(analysis): read R's NA as missing so parametric fits run --- CHANGELOG.md | 21 +++++++++++++++++++++ src/analysis/AnalysisConfig.jl | 5 ++++- src/analysis/Execution.jl | 4 ++-- src/analysis/estimation.jl | 33 ++++++++++++++++++++++++++++----- test/unit/test_estimation.jl | 15 +++++++++++++++ 5 files changed, 70 insertions(+), 8 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 51961d9a..5f299c79 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,27 @@ types, tests, infrastructure, and alignment. ## [Unreleased] +### Fixed — every parametric fit returned `not_run`; CI failures are now readable (2026-09-26) + +- **R's `NA` is read as missing.** The estimator writes its fits with + `write.csv(..., na = "NA")` and read them back with CSV.jl's default + `missingstring = ""`. One `NA` in a numeric column made the whole column a string + column, `isfinite` threw, and every NB/logistic/Gaussian fit came back as `not_run` + with no statistics. The Julia side now reads `"NA"` as missing (`R_NA_STRINGS`). + Julia tests on `main` had been red since #60 for this reason. +- **`glmGamPoi` reaches its by-name refusal.** The configuration lower-cases + `dispersion_method` but compared it against a table spelling `glmGamPoi`, so the name + was turned away as unknown and the issue #21 explanation was never shown. The compare + is now lower-case against lower-case, and the estimator looks its refusals up + case-insensitively. A test covers three spellings. +- **Source-scan tests no longer trip on comments.** Comments quoting the removed + hash-derived p-value code were reworded so the "no placeholder statistics" scan stays + strict without flagging its own history. +- **CI names the failing assertion.** The "Run tests" step posts one annotation per + `Test Failed` / `Error During Test` block and one with only the failing summary rows. + The old single annotation was cut to 4096 bytes by the checks API, which removed the + failing testset, and the job log is served from a host some environments cannot reach. + ### Fixed — the ILR path stops substituting a basis it was not asked for (2026-09-25) - **Unimplemented ILR bases are refused rather than substituted.** The configuration diff --git a/src/analysis/AnalysisConfig.jl b/src/analysis/AnalysisConfig.jl index 224f81dc..852d3293 100644 --- a/src/analysis/AnalysisConfig.jl +++ b/src/analysis/AnalysisConfig.jl @@ -335,7 +335,10 @@ struct AdvancedConfig zero_policy_clean = lowercase(strip(zero_policy)) # Dispersion method validation - if !(dispersion_method_clean in VALID_DISPERSION_METHODS) + # Lower-case against lower-case: the table spells `glmGamPoi` the way the package + # does, and comparing a lower-cased input against it refused the name at the door, so + # the estimator's by-name refusal (issue #21) was never reached. + if !(dispersion_method_clean in lowercase.(VALID_DISPERSION_METHODS)) throw(ArgumentError("dispersion_method must be one of $(join(VALID_DISPERSION_METHODS, ", ")) — got '$dispersion_method_clean'. See context_help('advanced.dispersion_method')")) end diff --git a/src/analysis/Execution.jl b/src/analysis/Execution.jl index 18458a0e..c512d044 100644 --- a/src/analysis/Execution.jl +++ b/src/analysis/Execution.jl @@ -1433,7 +1433,7 @@ end # a fit cannot be run. # # `Estimation.estimate_models` is the estimator. The block that used to stand here derived its -# p-values from `hash(taxon_id)`, which is a function of the feature name and not of the data: +# p-values from a hash of the taxon id, which is a function of the feature name and not of the data: # it has been removed rather than deprecated, and test/unit/test_estimation.jl asserts at the # source level that it does not come back. const ESTIMATION_R_WAIT_SECONDS = Ref(10.0) @@ -1575,7 +1575,7 @@ function run_analysis( # ---------------------------------------------------------------------- # Estimation — every number below comes from a fit that ran # - # This replaces a block that computed `p_val = 0.01 + (hash(taxon_id) % 100)/1000.0` + # This replaces a block that computed `p_val = 0.01 + (h % 100)/1000.0` with `h` the hash of the taxon id # and `padj = p_val * 1.5` and returned them as results. Those numbers were a # function of the feature NAME: reproducible, plausible-looking and meaningless. # See src/analysis/estimation.jl for what runs instead and diff --git a/src/analysis/estimation.jl b/src/analysis/estimation.jl index cebafcfe..851a67f0 100644 --- a/src/analysis/estimation.jl +++ b/src/analysis/estimation.jl @@ -9,7 +9,7 @@ # # This module exists because the layer it replaces did not compute anything. The block # it replaces in Execution.run_analysis derived its p-values from -# `p_val = 0.01 + (h % 100) / 1000.0` where `h = hash(taxon_id)` — a deterministic +# `p_val = 0.01 + (h % 100) / 1000.0` where `h` was the hash of the taxon id — a deterministic # function of the feature NAME, carrying no information about the counts. The source # called it a stub; the caller received it as a result. A stub that is returned as a # result is a wrong answer, so it is gone rather than deprecated. @@ -77,6 +77,28 @@ const REFUSED_DISPERSION = Dict{String,String}( const SUPPORTED_DISPERSION = ("parametric",) +""" +The strings R's `write.csv(..., na = "NA")` uses for a missing value. CSV.jl's default +`missingstring` is only `""`, so without this a numeric column holding one `NA` is read as a +string column and every later `isfinite` throws — which turned every fit into `not_run`. +""" +const R_NA_STRINGS = ["NA", ""] + +""" + _refused_dispersion(name) -> Union{Nothing,Pair{String,String}} + +Look a dispersion name up in `REFUSED_DISPERSION` without regard to case: the configuration +layer lower-cases what it stores, and a `glmgampoi` has to meet the same refusal, with the +same reason, as `glmGamPoi`. +""" +function _refused_dispersion(name::AbstractString) + key = lowercase(strip(name)) + for (k, v) in REFUSED_DISPERSION + lowercase(k) == key && return k => v + end + return nothing +end + """ ESTIMATE_SCALE @@ -385,8 +407,9 @@ function estimate_models(config::AnalysisConfig.AnalysisConfig, # -- method preconditions (configuration errors: hard refusals) ------- if method == AnalysisConfig.NB_GLM dispersion = lowercase(strip(config.advanced.dispersion_method)) - haskey(REFUSED_DISPERSION, dispersion) && - throw(ArgumentError("dispersion_method '$dispersion' is $(REFUSED_DISPERSION[dispersion]) See issue #21 and docs/statistics/method-conditions/parametric-fits.md.")) + refused = _refused_dispersion(dispersion) + isnothing(refused) || + throw(ArgumentError("dispersion_method '$(first(refused))' is $(last(refused)) See issue #21 and docs/statistics/method-conditions/parametric-fits.md.")) dispersion in SUPPORTED_DISPERSION || throw(ArgumentError("dispersion_method '$dispersion' is not one of $(join(SUPPORTED_DISPERSION, ", ")). Refusing.")) @@ -450,8 +473,8 @@ function estimate_models(config::AnalysisConfig.AnalysisConfig, "mass" => RCall.rcopy(String, RCall.reval("est_mass_version")), "primary_levels" => RCall.rcopy(String, RCall.reval("est_primary_levels_text")), ) - fit_rows[] = collect(CSV.File(fits_path)) - coef_rows[] = collect(CSV.File(coefs_path)) + fit_rows[] = collect(CSV.File(fits_path; missingstring = R_NA_STRINGS)) + coef_rows[] = collect(CSV.File(coefs_path; missingstring = R_NA_STRINGS)) RCall.reval("rm(list = ls(pattern = \"^est_\")); gc()") return nothing end diff --git a/test/unit/test_estimation.jl b/test/unit/test_estimation.jl index acc71d39..b0615d4d 100644 --- a/test/unit/test_estimation.jl +++ b/test/unit/test_estimation.jl @@ -182,6 +182,21 @@ @test occursin("glmGamPoi", err.msg) @test occursin("#21", err.msg) + # The configuration layer lower-cases what it stores; every spelling has to reach the + # same by-name refusal instead of being turned away as an unknown method at the door. + for spelling in ("glmgampoi", "GLMGAMPOI", " glmGamPoi ") + spelled = try + Estimation.estimate_models(nb_config(dispersion = spelling), counts_effect; + sample_metadata = meta_effect, offset = offsets_effect, + taxa_ids = taxa_effect) + nothing + catch e + e + end + @test spelled isa ArgumentError + @test occursin("glmGamPoi", spelled.msg) && occursin("#21", spelled.msg) + end + # Counting without an offset would compare library sizes instead of groups. @test_throws ArgumentError Estimation.estimate_models( nb_config(), counts_effect; sample_metadata = meta_effect, taxa_ids = taxa_effect)