fix: reject nulls at load time in every schema validator#26
Merged
Conversation
validate_gwas_dataframe already required non-null positions and p-values. validate_eqtl_dataframe, validate_finemapping_dataframe, and validate_genes_dataframe did not, so a file with a blank p_value, pip, start, or end passed load validation and silently lost those rows later, when plot-time intake filtered them with a warning the user was unlikely to connect to the file. The gap was an artefact of the old hand-rolled implementation rather than a decision; the 2.0 refactor preserved it deliberately to stay behaviour-neutral, and this is the follow-up that closes it. All four validators now reject nulls in the numeric columns they range-check, naming the offending column. Verified end to end through load_susie: a clean file loads, one with a blank pip fails with "Column 'pip' has 1 null values". Plot-time intake is untouched and still filters nulls with a warning. That split is the point: a frame reaching the plotter may have been assembled by the caller rather than read from a file. Recorded in CONTEXT.md alongside the other load-bearing intake distinctions.
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.
Closes the one follow-up left open by #25.
validate_gwas_dataframealready required non-null positions and p-values.validate_eqtl_dataframe,validate_finemapping_dataframe, andvalidate_genes_dataframedid not, so a file with a blankp_value,pip,start, orendpassed load validation and silently lost those rows later,when plot-time intake filtered them with a warning the user was unlikely to
connect back to the file.
That gap was an artefact of the old hand-rolled implementation rather than a
decision. #25 preserved it deliberately so the refactor stayed
behaviour-neutral; this is the labelled behaviour change that closes it.
Change
All four schema validators now reject nulls in the numeric columns they already
range-check, naming the offending column:
validate_eqtl_dataframepos,p_valuevalidate_finemapping_dataframepos,pipvalidate_genes_dataframestart,endvalidate_gwas_dataframeis unchanged; it was already strict.What is deliberately not changed
Plot-time intake stays lenient and still filters nulls with a warning
(
prepare_pvalue_data,prepare_eqtl_for_plotting). That split is the point: aframe reaching the plotter may have been assembled by the caller rather than
read from a file, so the strict gate belongs at the file boundary. Recorded in
CONTEXT.mdnext to the other load-bearing intake distinctions, and in thecontract test's module docstring.
Verification
Five new contract cases were red before the change and green after, and two
cases that asserted the old tolerance are flipped to rejections.
End to end through a real loader, not just the validator:
Full suite: 1195 passed, 1 skipped.
ruff checkandruff formatclean.Release note
2.0.0 is merged but not yet tagged, so this lands in the existing 2.0.0
CHANGELOG entry rather than a patch bump.