diff --git a/CHANGELOG.md b/CHANGELOG.md index 5613a37..102815a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,8 @@ All notable changes to this project are documented in this file. ### Fixed - **`number_of_cases` is now emitted as a JSON integer instead of source text.** The slot is typed `int` on biolink-model's `EntityToDiseaseAssociation` / `EntityToPhenotypicFeatureAssociation`, but it was absent from `lib.numeric_columns`' exact set — since 15.1's `STUDY_SIZE_EXEMPT_PATTERN` (#119) stopped it being *renamed* onto `study_size`, nothing cast its *values*, so the raw TSV cell (`"1"`) shipped on the edge NDJSON. Pydantic's lax validation coerced the string back to int inside `validate_kgx`, so the record still validated, and the Rust `uuid_on_collision: merge` recompute already wrote a real int — leaving the shipped graph type-inconsistent edge to edge. `number_of_cases` now rides the same `clean_numeric` / `format_numeric` machinery as `study_size`: `numeric_slot_kind` reads the `int` typing off the installed model, fractional / negative / non-numeric counts become null, and the release-mode `drop_low_number_of_cases` filter is untouched (it already cast inline). +- **`--release` builds no longer reuse section parquets cached by other build modes.** The section-parquet cache key was `STORE / f"{mkhash(s)}.parquet"` with `release` injected into `Tcode` outside the hashed dict, and `Tcode.collect` quick-exits on any existing store file — so a parquet cached by a non-release build (or a pre-16.6.0 build, where the case-count filter silently no-op'd) was reused verbatim in `--release` builds, skipping the release filters and shipping `applied_to_treat` edges with `number_of_cases < 25`. A new `_section_store_path` helper generalizes the existing `.head.parquet` idiom: `--head`, `--release`, and `--qc` each get their own suffix, composing when combined (`.head.release.qc.parquet`). `--qc` belongs because `fullmap_audit` drops rejects from the cached parquet too — the same cache-poisoning class. ([#140](https://github.com/SkyeAv/Tablassert/pull/140)) +- **The release-mode case-count and effect-size filters tolerate non-numeric cells instead of crashing.** `drop_low_number_of_cases` and `drop_zero_effect_size` cast with strict `Float64`, but the `csv` op reads sources with `has_header=False`, so a TSV's header row flows through as data and the strict cast raised `InvalidOperationError` on the non-numeric header cell. Both ops now cast with `strict=False` (the `clean_numeric` idiom); non-numeric cells become null and are kept, dying later at entity resolution as before. ([#140](https://github.com/SkyeAv/Tablassert/pull/140)) ## 16.6.1 - 2026-09-04 diff --git a/src/tablassert/cli.py b/src/tablassert/cli.py index 386eac8..4be74cc 100644 --- a/src/tablassert/cli.py +++ b/src/tablassert/cli.py @@ -49,6 +49,32 @@ BABEL_SYNONYM_RE: re.Pattern[str] = re.compile(r' Path: + """Name a section's cached subgraph parquet so build-mode flags never share a cache file. + + The cached parquet's content depends on the build mode: ``--head`` samples + rows, ``--release`` applies the release-mode significance filters, and + ``--qc`` drops fullmap-audit rejects. ``Tcode.collect`` quick-exits on any + existing store file, skipping the whole op chain, so each mode combination + must cache under its own suffix or one mode's build is silently reused as + another's. + + Args: + h: Section config hash (``mkhash`` of the section dict). + head: ``--head`` preview build flag. + release: ``--release`` build flag. + qc: ``--qc`` build flag. + + Returns: + Store path like ``.parquet`` with ``.head`` / ``.release`` / ``.qc`` + inserted before the extension for each set flag. + """ + from tablassert.utils import STORE + + suffix: str = (".head" if head else "") + (".release" if release else "") + (".qc" if qc else "") + return STORE / f"{h}{suffix}.parquet" + + def _load_table_indexed(args: tuple[int, Path]) -> tuple[int, object]: """Load one table, tagged with its input index (multiprocessing worker). @@ -162,7 +188,7 @@ def build_graph_pipeline( from tablassert.fullmap import fullmap_db_path from tablassert.lib import Tcode, compile_graph, compile_subgraph from tablassert.progress import flatten_pydantic_error, format_section_compact - from tablassert.utils import STORE, mkhash + from tablassert.utils import mkhash # Stage 1/6: load tables. progress.stage("Loading Tables") @@ -204,8 +230,9 @@ def build_graph_pipeline( for s in sections: h: str = mkhash(s) start(f"{Path(str(s['config'])).stem} · {h[:8]}") - # --head preview builds cache to a distinct .head.parquet so they never clobber full builds. - store: Path = STORE / (f"{h}.head.parquet" if head else f"{h}.parquet") + # Mode flags change the cached parquet's content, so each combination caches + # to a distinct file and can never quick-exit another mode's build. + store: Path = _section_store_path(h, head=head, release=release, qc=qc) try: tcode.append( Tcode.model_validate( diff --git a/src/tablassert/lib.py b/src/tablassert/lib.py index 8c47878..31f82d6 100644 --- a/src/tablassert/lib.py +++ b/src/tablassert/lib.py @@ -1046,12 +1046,16 @@ def drop_zero_effect_size(lf: pl.LazyFrame, col: str = "effect_size") -> pl.Lazy Notes: Only filters when the effect-size column exists; no-op for sections without an ``effect_size`` column. Null effect sizes are kept (no - score was detected for that row). + score was detected for that row). The cast is non-strict (like + ``clean_numeric``) and paired with ``ne_missing``: a non-numeric cell -- + e.g. a header row flowing through as data because the ``csv`` op reads + with ``has_header=False`` -- becomes null and is kept, dying later at + entity resolution instead of crashing the build with a strict-cast error. """ names: list[str] = lf.collect_schema().names() if col not in names: return lf - return lf.filter(pl.col(col).is_null() | (pl.col(col).cast(pl.Float64) != 0.0)) + return lf.filter(pl.col(col).cast(pl.Float64, strict=False).ne_missing(0.0)) def drop_low_number_of_cases(lf: pl.LazyFrame, col: str = "number_of_cases", threshold: float = 25.0) -> pl.LazyFrame: @@ -1068,13 +1072,18 @@ def drop_low_number_of_cases(lf: pl.LazyFrame, col: str = "number_of_cases", thr Notes: Only filters when the number-of-cases column exists; no-op for sections without a ``number_of_cases`` column. Null case counts are kept (no count - was detected for that row). Only wired in for ``applied_to_treat`` - sections at op-construction time, so it never touches other predicates. + was detected for that row). The cast is non-strict (like ``clean_numeric``) + with ``fill_null(True)``: a non-numeric cell -- e.g. a header row flowing + through as data because the ``csv`` op reads with ``has_header=False`` -- + becomes null and is kept, dying later at entity resolution instead of + crashing the build with a strict-cast error. Only wired in for + ``applied_to_treat`` sections at op-construction time, so it never + touches other predicates. """ names: list[str] = lf.collect_schema().names() if col not in names: return lf - return lf.filter(pl.col(col).is_null() | (pl.col(col).cast(pl.Float64) >= threshold)) + return lf.filter(pl.col(col).cast(pl.Float64, strict=False).ge(threshold).fill_null(True)) def idxname(col: Any) -> str: diff --git a/tests/test_lib.py b/tests/test_lib.py index af3ef65..95b93c9 100644 --- a/tests/test_lib.py +++ b/tests/test_lib.py @@ -342,6 +342,38 @@ def test_tcode_collect_omits_drop_low_number_of_cases_for_other_predicates_in_re assert "drop_not_significant" in names +def test_section_store_path_suffixes_every_build_mode() -> None: + """_section_store_path gives each build-mode flag combination its own cache file name.""" + assert cli._section_store_path("abc123").name == "abc123.parquet" + assert cli._section_store_path("abc123", head=True).name == "abc123.head.parquet" + assert cli._section_store_path("abc123", release=True).name == "abc123.release.parquet" + assert cli._section_store_path("abc123", qc=True).name == "abc123.qc.parquet" + # Modes compose instead of overwriting one another's suffix. + assert cli._section_store_path("abc123", head=True, release=True, qc=True).name == "abc123.head.release.qc.parquet" + + +def test_tcode_collect_quick_exit_never_serves_another_mode_cached_parquet(monkeypatch: Any, fixtures_path: Path, tmp_path: Path) -> None: + """A parquet cached by a non-release build never quick-exits a release build (and vice versa).""" + # STORE is relative to the cwd, so chdir keeps the simulated cache inside tmp_path. + monkeypatch.chdir(tmp_path) + (tmp_path / ".tablassert" / "store").mkdir(parents=True) + data: Any = from_yaml(fixtures_path / "minimal_section.yaml") + # A previous non-release build cached this section's subgraph (filters skipped). + pl.DataFrame({"subject": ["cached"]}).write_parquet(cli._section_store_path("sectionhash")) + store: Path = cli._section_store_path("sectionhash", release=True) + tcode_model: Tcode = Tcode.model_validate( # pyright: ignore + {**data, "config": fixtures_path / "minimal_section.yaml", "store": store, "release": True} + ) + + collected: Any = tcode_model.collect(tmp_path / "fullmap.redb") + + # The release store does not exist yet: the non-release parquet must not be reused. + assert not isinstance(collected, Path) + # Once the release-mode build has written its own parquet, the quick exit serves it. + pl.DataFrame({"subject": ["release"]}).write_parquet(store) + assert tcode_model.collect(tmp_path / "fullmap.redb") == store + + def test_tcode_model_validate_rejects_duplicate_qualifier_keys(fixtures_path: Path) -> None: """Tcode construction rejects a section whose statement repeats a qualifier key. @@ -1292,6 +1324,22 @@ def test_drop_low_number_of_cases_noop_without_column() -> None: assert list(result["subject"]) == ["a", "b"] +def test_drop_low_number_of_cases_keeps_non_numeric_cells() -> None: + """drop_low_number_of_cases keeps non-numeric cells (a header row read as data) instead of crashing on a strict cast.""" + # The csv op reads sources with has_header=False, so a TSV's header row flows + # through as a data row; the strict cast raised InvalidOperationError on it. + lf: pl.LazyFrame = pl.DataFrame({"subject": ["hdr", "a", "b", "c"], "number_of_cases": ["number_of_cases", "30", "10", None]}).lazy() + result: pl.DataFrame = drop_low_number_of_cases(lf).collect() + assert list(result["subject"]) == ["hdr", "a", "c"] + + +def test_drop_zero_effect_size_keeps_non_numeric_cells() -> None: + """drop_zero_effect_size keeps non-numeric cells (a header row read as data) instead of crashing on a strict cast.""" + lf: pl.LazyFrame = pl.DataFrame({"subject": ["hdr", "a", "b", "c"], "effect_size": ["effect_size", "0.0", "1.5", None]}).lazy() + result: pl.DataFrame = drop_zero_effect_size(lf).collect() + assert list(result["subject"]) == ["hdr", "b", "c"] + + def test_numeric_columns_matches_p_value_substring() -> None: """numeric_columns matches any column with P value in the name.""" names: list[str] = ["p_value", "adjusted_p_value", "log_p_value", "subject"]