Add advection velocity to species mapping + 'user defined' parameter role - #48
Merged
Conversation
Repairs three issues introduced when these cherry-picks were rebased onto main: - SpeciesMapping moved to models_app.py in the data-model split, so the rebase dropped the velocity_x/_y/_z fields the reader/writer rely on — restore them (and include velocity in `expressions` for field-data extraction). Without this, parsing any VCML with a <Velocity> element raised on an unknown field. - The generic_visit -> generic_visit_children rename missed visit_Parameter (vcml_reader.py:212); the stale call was masked by the best-effort backstop (caught AttributeError per parameter). Fix the call site. - Register the new SASCO fixtures (vcml_sasco_model_path, vcml_sasco_model_with_velocity_path) in conftest so the new tests resolve them. Also tidy test_results_writing (drop debug prints/dead code, add a channel_data assertion, cleanup in finally). Velocity round-trips through the reader/writer; existing reader/writer round-trip equality is preserved (no velocity in those fixtures). make check green; new SASCO simulation tests pass (incl. the 21-velocity-channel assertion); full vcml suite 43 passed / 5 skipped. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Merged
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.
Cherry-picked from @LDre398 (Logan Drescher), then reviewed, fixed (rebase gaps), tested and linted.
What's in here
Advection velocity on species mappings — round-trips a
<Velocity X= Y= Z=>element underLocalizedCompoundSpec:SpeciesMappinggainsvelocity_x/_y/_z(float | str | None), included inexpressionsfor field-data extraction;visit_Velocityparses the element; writer emits it, writing only non-"0.0"components.Parameter role —
Model.add_model_parameter(...)now defaultsrole="user defined"(and accepts an override), matching VCell's Java representation (was"model_parameter").Reader method rename —
generic_visit→generic_visit_children(clearer intent), updated across the visitor.New SASCO fixtures + tests exercise the feature end-to-end (load → write → simulate → plot), including a velocity model that yields 21 velocity output channels.
Review notes — rebase gaps I fixed
These cherry-picks were authored before the data-model split (#41) and the lenient-reader work (#44), so the rebase left three breakages:
SpeciesMappingmoved frommodels.pytomodels_app.py, so the rebase lost thevelocity_x/_y/_zfield additions. The reader'snode.velocity_x = …would raise on a pydantic model with no such field — i.e. any VCML with a<Velocity>element failed to load. Restored the fields.generic_visit→generic_visit_childrenmissedvisit_Parameter(vcml_reader.py:212); the stale call was silently swallowed by the best-effort backstop (a caughtAttributeErrorper<Parameter>, plus dropped nested-parameter children). Fixed the call site.model_fixtures.pybut not imported inconftest.py, so the new tests errored with "fixture not found". Registered them.Also tidied
test_results_writing.py(removed debugprints / commented-out dead code, added achannel_dataassertion, runcleanup()infinally).Verification
make checkgreen — ruff, mypy (312 files), deptry,uv lock --check.len(velocity_channels) == 21); fulltests/vcmlsuite → 43 passed, 5 skipped.Note
The SASCO tests run real FV simulations (small 16×31×1 2D mesh, EndTime=10 — ~50s total), so they add some CI time but validate the velocity output end-to-end.
🤖 Generated with Claude Code