Fix validSim() passing the resource spectrum as the other components - #601
Merged
Conversation
`validSim.MizerSim()` diagnoses a simulation that went non-finite by
recomputing the rates at the last good time step. It passed
`sim@n_pp[max_t_idx, ]` for `n_other`, so any rate function reading
`n_other$<component>` was handed the resource number density instead of
the list of other components and failed with
$ operator is invalid for atomic vectors
Use `get_sim_rate_slice()`, which already builds exactly this slice and
restores the component names that `[` drops when a model has only one
component. It also supplies the dimnames on `n` that the hand-rolled
`dim(n) <- ...` left off.
The bug was invisible to the test suite because the shared fixtures have
no components, so nothing read `n_other`. The regression test therefore
builds a one-component model with `setComponent()` and checks both that
the diagnosis path completes and that the rate function received a named
list holding the stored component value.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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.
What changed
validSim.MizerSim()diagnoses a simulation that went non-finite by recomputing the rates at the last good time step. ThegetRates()call passedsim@n_pp[max_t_idx, ]for then_otherargument — a copy-paste slip that handed the resource number density to the rate functions where they expect the list of other components.Any rate function reading
n_other$<component>therefore failed with:Rather than just swapping the slot, the call now uses the existing internal helper
get_sim_rate_slice()(R/sim_rates.R), which already builds exactly this slice. That matters because a plainsim@n_other[max_t_idx, ]is not enough on its own: on a list-matrix with a single component,[drops to an unnamed length-1 list, son_other$<component>would come backNULLfor a one-component model. The helper restores the component names fromdimnames(sim@n_other)$component. It also supplies the dimnames onnthat the hand-rolleddim(n) <- dim(sim@n)[2:3]left off.Why it was invisible to the test suite
The shared fixtures have no other components, so nothing in the suite ever read
n_otheron this path. It surfaced when the suite was run withNS_params_smallreplaced by amizerSeasonalobject, which adds agonadscomponent —test-MizerSim-class.R("validSim works") then errored as above.Notes for the reviewer
validSim.MizerSim()has a singleif (!all(is.finite(sim@n)))block;is.finite()is already false forNaNas well asInf/-Inf, and the existing test's second half exercises theNaNcase through that same branch. There is no separate NaN or negative-value branch in the file —grepforis.finite|is.nan|negative|< 0inR/MizerSim-class.Rreturns only the three lines of this block.@n_other(get_sim_rate_slice()itself andfinalNOther()) already restore the names correctly, so nothing else needed fixing.upgrade-mizer-codeentry: this turns a crash into working diagnostics and moves no results for code that was already working.Tests
A regression test goes in
tests/testthat/test-MizerSim-class.R, the file named afterR/MizerSim-class.Rwhich definesvalidSim(per.claude/skills/test-organisation.md). It builds on the sharedNS_params_smallfixture plus a one-componentsetComponent()model whosemort_funreadsn_other$gonadsand records what it was handed, then asserts the truncation happened, thatnames(n_other)is"gonads", and that the value matches the one stored at that time step — so it catches both the wrong-slot slip and a names-dropping regression.Verified the test reproduces the reported error against the unfixed code, and passes with the fix.
devtools::test(filter = "MizerSim-class")— 61 pass, 0 faildevtools::test(filter = "extension|sim_rates|project|summary_methods")— 515 pass, 0 failNEWS.mdgets an entry under Extensions.🤖 Generated with Claude Code