Read dirichlet csvs with the substrate csv machinery

[drbergman]

Rewritten after #69 landed on my-physicell. The previous revision of this branch hardened dirichlet_csv_to_vector in place and added a store_dirichlet_csv_entry helper; both are gone. Rebased on c7ff0946.

#69's substrate_csv_to_vector returns the 1-based index of the first malformed field and yields NaN for an omitted entry:

// returns the 1-based index of the first malformed field, or 0 if the row is well formed;
// an empty field yields NaN so that an omitted entry is distinguishable from a zero
unsigned int substrate_csv_to_vector(const char* buffer, std::vector<double>& vect);

That NaN sentinel is precisely what the parallel is_missing vector existed to carry, so the dirichlet reader now uses the same parser and dirichlet_csv_to_vector is deleted outright.

Rather than copy the header handling a third time, the two loaders now share it. resolve_substrate_csv_columns reads the optional x,y,z,<name>,... header, applies the headerless "first n densities" convention, and leaves the stream on the first data row. The readers differ only in what a parsed row does to the microenvironment:

// substrate: an omitted entry resolves to 0
if (std::isnan(value)) { value = 0.0; }
microenvironment.density_vector(voxel_ind)[substrate_indices[ci]] = value;

// dirichlet: an omitted entry leaves that voxel-substrate pair alone
if (std::isnan(data[ci + 3])) { continue; }
microenvironment.update_dirichlet_node(voxel_ind, substrate_indices[ci], data[ci + 3]);
microenvironment.set_only_substrate_dirichlet_activation(substrate_indices[ci], true);

Net −65 lines.

Sharing it fixes the headerless dirichlet path

The dirichlet loader carried both bugs #69 fixed on the substrate side: if (i<3) {continue;} jumped past the i++ below it, so substrate_indices came out empty; and the reopened std::ifstream file(filename, ...) shadowed the enclosing stream that had just been closed, so the row loop read a closed stream.

A headerless dcs.csv therefore set no dirichlet conditions at all, silently. Measured on the sample, final microenvironment md5:

                        headered dcs.csv   headerless dcs.csv   dirichlet file disabled
  before (c7ff0946)     3a667848…          de131f4d…            de131f4d…
  after                 3a667848…          3a667848…            de131f4d…

Before, headerless was byte-identical to not supplying the file. After, it matches the headered file — which is the correct answer, since x,y,z,substrate,oxygen and a headerless two-value row both map to densities 0 and 1.

The stock sample is unaffected

sample_projects/dirichlet_from_file/config/dcs.csv leans on empty fields meaning "leave this pair alone" on nearly every row, so this is the case that mattered most. Byte-identical before and after:

before: init=7e4755eb…  final=1152b7fb…
after:  init=7e4755eb…  final=1152b7fb…

Verification

13 variants of each csv through the dirichlet_from_file sample with substrates.csv wired into <initial_condition type="csv"> alongside dcs.csv, all under -D_GLIBCXX_ASSERTIONS. All nine well-formed variants of both files load clean with no assertion failures; the four malformed ones exit non-zero. Diagnostics are now symmetric across the two readers and name the exact line and column:

ic  too_many      ERROR : Line 3 of the .csv … initial conditions has the wrong number of columns.
ic  not_a_number  ERROR : Column 4 of line 3 of the .csv … initial conditions is not a number.
dc  too_many      ERROR : Line 3 of the .csv … dirichlet conditions has the wrong number of columns.
dc  not_a_number  ERROR : Column 4 of line 3 of the .csv … dirichlet conditions is not a number.

The out-of-bounds write flagged in the first revision — dirichlet_csv_to_vector incrementing ind once per comma with no bound — disappears with the function. template also builds and runs unchanged.

One thing to weigh

Extracting resolve_substrate_csv_columns touches the substrate loader that #69 just landed, and MathCancer#386 carries that same code upstream. So this creates a small conflict surface with MathCancer#386 — resolvable, but real. If you'd rather keep my-physicell textually close to MathCancer#386, say so and I'll leave the substrate loader alone and give the dirichlet reader its own copy of the header handling instead. I went the sharing route because two divergent near-copies of this function is what lost trim_cr in the first place.

🤖 Generated with Claude Code

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[drbergman]

is this how we checked for blank lines elsewhere? either in my-physicell or in the newly merged code into development?

[drbergman]

No — and deliberately not. Elsewhere we check the parsed result's size; here I check the raw line before parsing. The two existing checks are:

• load_cells_csv_v1: if (data.size() == 0) { continue; } after csv_to_vector
• process_csv_v2_line: if (tokens.size() == 0) { return NULL; } after tokenizing

Neither idiom works on this function. I probed both parsers directly, with the test string placed so its terminator is the last readable byte before an unmapped guard page (so a read past the end faults deterministically rather than wandering the heap):

                        input          substrate_csv_to_vector   csv_to_vector
  ""      (blank line)                 size=1  NOT skipped       size=0  skipped
  " "     (one space)                  size=1  NOT skipped       SIGBUS in parser
  "   "   (three spaces)               size=1  NOT skipped       SIGBUS in parser
  "\t"    (tab)                        size=1  NOT skipped       SIGBUS in parser
  "0,0,0,1" (control)                  size=4                    size=4

Two separate problems with checking after the parse:

1. On this branch the check would be dead code. substrate_csv_to_vector always emits a final field, so a blank line comes back as size=1, never 0. data.size() == 0 never fires, and data[1]/data[2] still read off the end — which is the bounds assertion in the PR body.
2. Even where csv_to_vector is used, the parse is what's unsafe. Its churn loop (while (isdigit(buffer[i]) == false && ...) { i++; }) has no bound on i, so on a line with no numeric character it walks straight past the terminator. Any size check after that call has already lost. That is the SIGBUS above, and it is why the feature-dc-init-file side of this segfaulted rather than silently misparsing.

Worth knowing while we're here: cells.csv doesn't actually handle whitespace-only lines either. process_csv_v2_line tokenizes " " into one token, so it falls past the == 0 check into tokens.size() < 4 and hard-exits with "expects at least 4 columns". So find_first_not_of(" \t\r") is strictly more tolerant than what MathCancer#421 gave cells.csv, not a gratuitous deviation from it.

If you want them consistent, the direction I'd suggest is moving the geometry checks onto the raw-line test rather than moving this one onto data.size(), since only the raw-line test runs before the unsafe parse. Happy to do that as its own PR — it changes cells.csv behaviour (whitespace-only lines would start being skipped instead of exiting), so it shouldn't ride along in here.

Short version: not the same check, because the existing one is a no-op against substrate_csv_to_vector and too late against csv_to_vector.

[drbergman]

Moot now — #69 landed this exact check on the substrate reader, so it isn't mine to defend any more:

if (line.find_first_not_of(" \t\r") == std::string::npos)
{ return; } // skip blank lines

The branch is rebased on c7ff0946 and my substrate-side hunk is gone. The dirichlet reader mirrors #69's check, which is the answer to your other comment: same machinery, not a third copy.

The cells.csv observation stands on its own if you want it later — process_csv_v2_line tokenizes " " into one token, so a whitespace-only line hard-exits there with "expects at least 4 columns" instead of being skipped. Separate PR, since it changes cells.csv behaviour.

[drbergman]

this is a new function...why do we need this? if we're just reading the concetnration at this (voxel,substrate) tuple, we should apply teh same thing to substrates. but I really don't think we do. can't we just borrow the machinery in place there? you can look through the MathCancer#386 for what will be happening there soon

[drbergman]

You're right, and MathCancer#386 already has the machinery. Its substrate_csv_to_vector does exactly this job:

// returns the 1-based index of the first malformed field, or 0 if the row is well formed;
// an empty field yields NaN so that an omitted entry is distinguishable from a zero
unsigned int substrate_csv_to_vector(const char* buffer, std::vector<double>& vect);

That NaN sentinel replaces the whole parallel is_missing vector, and the returned index replaces my "column N is not a number" path. Once MathCancer#386 lands, store_dirichlet_csv_entry and dirichlet_csv_to_vector should both be deleted, and get_row_from_dirichlet_condition_csv should call substrate_csv_to_vector and test std::isnan(data[ci + 3]) where it currently tests is_missing[ci + 3]. One parser, one convention, ~90 fewer lines than today. I should have gone looking for that instead of hardening the copy in front of me.

The blocker is only ordering. On my-physicell as it stands, substrate_csv_to_vector still maps an empty field to 0.0, so it cannot express "no DC here". Fed a real row from the sample project's dcs.csv, where the trailing empty field means don't set a DC for oxygen in this voxel:

input: "-210,-390,0.0,78.0,"

my-physicell today:  size=5  -210, -390, 0.0, 78.0, 0.000000   <- omitted entry reads as 0
#386:                size=5  -210, -390, 0.0, 78.0, NaN        <- omitted entry stays distinguishable
                             bad_field=0

Reusing it here would turn every omitted entry into an explicit Dirichlet value of 0 and switch on set_only_substrate_dirichlet_activation for it. sample_projects/dirichlet_from_file/config/dcs.csv depends on that distinction on nearly every row, so it would quietly change what that sample simulates.

So my suggestion: narrow this PR to the parts that don't need the new parser, and do the unification as a follow-up once MathCancer#386 is in. Concretely, keep

• the two blank-line skips (the out-of-bounds reads), and
• the ind >= is_missing.size() bounds check (the out-of-bounds write on an over-long row)

and drop the field trimming and complete-number validation from this PR, since MathCancer#386 gives both for free. What defers with them is small and not memory-unsafe: std::stod still throws uncaught on a whitespace-only field like 0,0,0, ,5, and "1.5abc" is still silently accepted as 1.5. Both disappear the moment the readers share the one parser.

Say the word and I'll push the narrowed version; or if you'd rather not carry an interim state at all, I'll close this and put up the unified change on top of MathCancer#386 instead. Either way the new function goes away — you're right that it shouldn't exist.

[drbergman]

Done — you were right, and #69 gave me the machinery to do it properly. Force-pushed; PR body rewritten.

dirichlet_csv_to_vector is deleted, store_dirichlet_csv_entry never existed, and the dirichlet reader now calls substrate_csv_to_vector and tests std::isnan(data[ci + 3]) where it used to test is_missing[ci + 3]. I also pulled the header handling out into one resolve_substrate_csv_columns that both loaders call, rather than giving the dirichlet side a third copy of it. Net −65 lines.

The payoff beyond deleting code: the dirichlet loader had both bugs #69 fixed on the substrate side — the continue that skipped i++, and the reopened ifstream shadowing the closed stream — so a headerless dcs.csv was setting no dirichlet conditions at all, silently. Final microenvironment md5 on the sample:

                     headered      headerless    file disabled
  before             3a667848…     de131f4d…     de131f4d…
  after              3a667848…     3a667848…     de131f4d…

Sharing the header code fixed that for free.

Stock sample is byte-identical before and after (init=7e4755eb… final=1152b7fb…), which was the case I was most worried about since its dcs.csv uses empty fields on nearly every row.

One judgement call flagged in the PR body: extracting the shared function touches the substrate loader #69 just landed, which MathCancer#386 also carries upstream, so there's a small conflict surface with MathCancer#386. Happy to back that out and give the dirichlet reader its own copy of the header handling if you'd rather keep my-physicell textually close to MathCancer#386 — I went the sharing route because two divergent near-copies is what lost trim_cr to begin with.