Repository navigation
Refuse plane_histograms.csv rows whose plane_id, image_file or bit_depth disagree - #19
Merged
Merged
Conversation
…pth disagree read_plane_histograms recomputed these three columns from source_relative_path, z, t and dtype and never compared them with the stored values, so an edited or merged table with a mismatched row key was accepted silently. The reader now raises CsvFormatError naming the line, column, stored and implied values. The table format and schema version are unchanged. Fixes #18 (from CellMigrationLab/morphobricks-preprocessing#39). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VbytvBRB9CmKQ5AAkhEFcv
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.
Fixes #18. This is the "CSV columns ignored" item of CellMigrationLab/morphobricks-preprocessing#39. Preprocessing PR CellMigrationLab/morphobricks-preprocessing#44 left it out because the reader lives in this repository.
What changed
csv_io._record(used byread_plane_histograms): after it builds thePlaneRecord, it compares the storedplane_id,image_fileandbit_depthwith the values implied bysource_relative_path,z,tanddtype(record.plane_id,record.image_file,bit_depth(dtype)). The comparison is exact text, which is howwrite_plane_histogramswrites them. A mismatch raisesCsvFormatError: "<path>, line N: its <column> is '<stored>', but its other columns give '<implied>'. The table was changed after it was written; run the preprocessing brick again on this experiment to rewrite it." Before this change, the stored values were dropped and recomputed, so an edited or merged row with another row key was accepted without any error.schema_version(4) do not change. Every table written bywrite_plane_histogramspasses the check, because the writer derives these columns the same way.Tests
tests/test_csv_io.py::test_derived_plane_columns_must_match_the_row(4 cases:plane_idnaming another file,plane_idnaming another z,image_file,bit_depth).DID NOT RAISE.pip install -e ".[dev]"): 474 passed, 2 skipped. The 2 skips aretests/test_rgb.py:78, becausecsbdeepis not installed (CI's separate job covers it).ruff check .: all checks passed.ruff format --check .: 38 files already formatted.Release and the preprocessing pin
morphobricks-normalization @ git+…@v0.3.1, so it does not get this fix until a new tag exists. I suggestv0.3.2, a patch release: the change only adds a refusal and the format is unchanged. As with earlier releases, that needs a release PR that setspyproject.tomlto0.3.2and updates the README/plan release notes, then the owner's tag. This PR does not change the version.@v0.3.1to@v0.3.2(the new tag) before it can close [Sad Catapella N3][Validation] Low-level normalization.apply accepts reversed clipping and integer output dtypes silently #39. If preprocessing v0.3.2 is tagged before that, it still ships normalization v0.3.1 without this check. Within preprocessing, the risk is small in the meantime: reused tables are guarded by the manifest'scsv_sha256.Generated by Claude Code