improve substrate csv initial conditions: omit voxels, fix out-of-bounds reads, revive headerless path - #386
Open
drbergman wants to merge 3 commits into
Open
Conversation
…quire well-formed rows (#66) * Bounds-check substrate IC csv rows before indexing them get_row_from_substrate_initial_condition_csv indexed data[0..2] and data[ci + 3] without consulting data.size(). Both reads are out of bounds for a malformed row: - substrate_csv_to_vector always emits a final field, so a blank line parses to a single 0.0 and data[1]/data[2] read past the end. - substrate_indices is sized from the header's column count, so a row with fewer columns than the header runs data[ci + 3] off the end. Reproduced with libc++ bounds checking on a 4-voxel microenvironment: a trailing blank line, a whitespace-only line, a CRLF blank line, a 2-column row, and a row supplying 1 of 3 header substrates all abort with "vector[] index out of bounds" before this change and are handled cleanly after it. Note these reads are not new here -- development crashes on the same five inputs. Its coverage check ran after the read loop, so it never guarded them. Blank lines are now skipped, matching load_cells_csv_v1 and process_csv_v2_line, and a short row is a hard error rather than a silent partial write. Ordering the guards ahead of the existing warning also stops data.size() - 3 from underflowing on a short row. Also correct that warning: it reported number_of_voxels() where it means number_of_densities(), and the block comment still described the one-row-per-voxel, no-header format this branch replaced. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Revive the headerless csv path and require well-formed rows The headerless branch of load_initial_conditions_from_csv could never have worked. Two bugs had to be fixed together to see either one: - "if (i<3) {continue;}" jumped past the "i++" below it, so i never advanced and substrate_indices came out empty. - the reopened "std::ifstream file(filename, ...)" shadowed the enclosing stream, which had just been closed, so the row loop read a closed stream and processed no rows. -Wshadow flags this. A headerless csv therefore loaded nothing. On development that at least failed loudly, because the deleted coverage check saw voxel_set.size() == 0 and exited; without it the run continued silently on the config file's uniform initial conditions. Reading the first row's column count and rewinding the stream fixes both, and the row loop now counts lines so every diagnostic can name the row it rejected. Rows are now held to being well formed rather than parsed as far as they go: - a field must be empty or a complete finite number. strtod's endptr was discarded, so "NA" and "1.5abc" silently became 0 and "inf" became a density. - a row must have exactly the column count the header (or the first row) established. Extra columns were silently dropped. - x, y and z must all be present, so ",,,," no longer resolves to the origin. - a position must lie inside the domain, since nearest_voxel_index clamps and would otherwise snap a typo onto an edge voxel. - a header may not name the same substrate twice, and must name at least one. - an empty file is an error rather than a header sniff on nothing. An omitted entry now travels as NaN instead of 0, which is what lets the row loop tell "the user left this blank" from "the user asked for zero". It still resolves to 0, as before; the distinction only makes the checks above possible. The header sniff also no longer indexes line.c_str()[2] and [4] without knowing they exist -- a first line of "x" read past the end. It splits the row and compares fields instead, which also makes the sniff whitespace tolerant. Verified against real BioFVM on a 2x2x1 microenvironment with libc++ bounds checking: 36 cases covering header/headerless, subsets, reordering, whitespace, CRLF, blank lines, and every rejection above. All 36 behave as intended with no out-of-bounds reads. Headerless files load correctly for the first time. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
Collaborator
Author
|
Update: folded in drbergman#66. Beyond the omitted-voxels feature, this now fixes correctness and robustness problems that are present on Measured against
|
drbergman
changed the title
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.
Currently, every voxel needs to be assigned a substrate concentration for every substrate column. Any substrates without a column are initialized as 0 everywhere, i.e., they ignore the XML.
Now, this PR allows for omitting voxels and having missing values in a row. Any missing voxel or missing value are set to 0. Thus,
sub1to 0 everywhere except (20,0,0) where it is 1sub2to 0 everywhere except (0,20,0) where it is 1Any other substrate would be 0 everywhere.