Repository navigation
fix(am): record every experiment's trajectory, normalized once; refuse what would be wrong (#990) - #994
Merged
Conversation
…e what would be wrong (#990) - Trajectory buffers are keyed by each experiment's output suffix (conditions and edition-1 mutants included) and allocated from the result, instead of by the action suffixes in time_length, which a conditioned experiment does not have. Edition-2 action x condition pairs that are no experiment are left out. - The recorded data is read wherever the objective scored it, and is not normalized a second time: output_trajectory holds the normalized series the objective scores. This also stops am failing on a chain whose first simulation failed, and under local_objective_eval = 1 or smoothing > 1. - Refused at load: a trajectory name no model or measurement formula defines (with the names it could match); either output on a column normalized with scale; output_noise_trajectory on a normalized column; normalization = scale on a noise family with no additive scale (neg_bin); and either output under any job type but am. A name no experiment produces, two experiments that would share a file, and an output whose length changes stop the run at the first simulation that shows it. - An empty ChainRecord comes back from a pickle able to take an append, so -r no longer stops with IndexError when an mh or pt chain had no result at the backup. - The dream / p_dream benchmark examples drop the trajectory keys they could not honour, and the pt409 example's lnVV typo is corrected to lnV.
This was referenced Oct 15, 2026
…#990, #998) - The job-type refusal of output_trajectory / output_noise_trajectory moves from configuration loading to the start of a fresh run, so a finished run whose conf set them under dream still loads and reads back as before. - from_pybnf raises, naming the cause, when it cannot recover the parameter scales (no .conf, one that does not load, an undeclared sampled parameter), instead of returning log parameters in natural space. - The noise-under-normalization refusal reads only the compiled per-suffix normalization, not the legacy per-file keys kept beside it, and names the experiment. - A result without a finite score records nothing and runs no name check, so the master- and worker-scoring paths end alike. - Two experiments that would share a trajectory file are refused when the configuration loads, from the columns each is sure to have. - -r from a backup made before #990 says which experiments' files start late. - The docs state the _Cum rule of output_trajectory as it is. - find_t_length, _load_t_length and config['time_length'] are removed: am no longer reads the length, and every job type was refused valid simulate actions computing it. Closes #998
The bridge read n_steps and t_end with defaults of its own (100 and 100), so n_output_steps=>20 gave 101 rows where BNG2.pl writes 21, a step-less action 101 where BNG2.pl writes 2, and an action without t_end ran to t = 100 where BNG2.pl refuses it. find_t_length used to refuse the first two at load by accident; removing it exposed the mismatch. - New pybnf/bngsim_model/grid.py applies BNGAction.pm's rules on every path (the .net simulate and protocol paths, NFsim simulate, network and NFsim scans): n_output_steps is n_steps, one step by default, n_steps ahead of sample_times, 3 or more sample_times, t_end required without them. On NFsim, n_steps and t_end > t_start are required, except for the sample_times bngsim's NFsim takes. - An action BNG2.pl refuses is refused with BNG2.pl's message when the fit sets up a bngsim model, outside the fall-back to BNG2.pl, and again when it runs. - Edition-2 synthesized actions name sample_times or n_steps and t_end, so they are unchanged: an edition-2 am fit over every experiment shape (ODE, SSA and NFsim) writes byte-identical results before and after.
This was referenced Oct 15, 2026
…the #872 exception (#998) - An action naming argfile, max_sim_steps, output_step_interval, or print_on_stop other than 1 is classified unsupported by the bngsim bridge: BNG2.pl reads these to decide its output rows, and the bridge does not. auto now runs such a model on BNG2.pl, and bngl_backend = bngsim refuses it, naming the option. An argfile action was refused with the claim that BNG2.pl refuses it, which it does not. - grid.py and the bngl_backend docs no longer claim the same grid on both backends without qualification: sample_times that start after t_start are still integrated from the first sample time on bngsim (#872, open). - Adds the re-review's grid tests, with the #872 case marked xfail (strict).
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.
Closes #990. Closes #998. These are the
amtrajectory-output defects found while reviewing #988.What changes
epi2hi), frommodel.get_suffixes(), instead of by the action suffix.experiment:line names are left out.time_lengthno longer decides it._keys_of, from fix(am): draw output_noise_trajectory from each column's own noise model (#986) #988), soepicannot catchepi2.amis already normalized by the scoring path, sooutput_trajectorynow records exactly what the objective scores, without the second normalization that crashed. Two combinations are refused at load:output_noise_trajectoryon a normalized column, because a count drawn around a normalized prediction has no meaning;scale, because that factor is applied inside the objective and is not in the simulated series.normalization = scaleon a noise family with no additive scale is refused at load (Configuration._require_additive_scale). The check reads the family'sadditive_on, not its class name. Of gaussian/normal, lognormal, lnnormal, laplace, student_t andneg_bin, onlyneg_binlacks one.'FailedSimulation' object has no attribute 'out'crash is fixed. A genuine failure can reach that line, because a chain's first result is always accepted.local_objective_eval = 1andsmoothing > 1, which crashed everyamrun that wrote trajectories ('Result' has no attribute 'out').output_trajectory/output_noise_trajectoryname is an error at load, naming the unmatched name and the observables, functions and species it could have matched.postprocess).ChainRecordsurvives a pickle round trip.mhandptcan back up a slow chain before its first result.-rthen raisedIndexErrorat that chain's first result.amcannot reach this, because of its generation barrier.Behaviour changes worth knowing
amnow refusesoutput_trajectoryandoutput_noise_trajectoryat load. They were silently ignored before.dream/p_dreamconfigs inexamples/sampler_benchmarkingset them; those lines are removed.examples/HIVdynamics_aMCMC/pt409/pt409.conf:lnVVis nowlnV.Sibling paths
am, worker scoringam, master scoring (local_objective_eval = 1)amwithsmoothing > 1/parallelize_models > 1get_suffixes; not run-r)mh,pt,dream,p_dream,de,checkChainRecordTests and oracles
Of the 36 new or rewritten tests, 31 fail on
main. The other 5 cover noise families that were already accepted.(exp(-k t) + 0.1)/1.1underfloor 0.1, init, edition 2, with a conditioned experiment;CumInf/maxand0.6*CumInffor an edition-1 mutant.Files run, one at a time, on the bngsim build from bngsim
main:test_adaptive_mcmc115test_basic_mcmc_class+test_bayesian_diagnostics86test_config_class112test_config_unused_keys187test_edition75test_lwf_config82test_noise_model_config67test_hmc52test_linear_profiling79test_dream_class23test_seed_determinism23test_sampler_integration13All passed.
ruffandtools/changelog.py checkare clean.Not covered here (to be filed)
excepts turn configuration errors into failed simulations. This is how thescaleAttributeErrorbecame penalties:pybnf/algorithms/core.py:407andpybnf/algorithms/base.py:948-953.time_errorignoresscaleandcumulative. Found by reading; not reproduced.scaleis not refused for Bayesian samplers, althoughnoise_profilingandlinear_profilingare.Review
Two independent review rounds, then a short check of the final commit:
from_pybnf's broadexcept;find_t_length(find_t_length refuses valid BNGL simulate actions for every job type (n_output_steps, no step count), misreads n_steps+sample_times and skips commented lines, to compute a length only am reads #998) exposed an older problem. The bngsim bridge ignoredn_output_stepsand defaulted to 100 output steps, where BNG2.pl defaults to 1. Withn_output_steps=>20, BNG2.pl writes 21 rows and the bridge wrote 101.pybnf/bngsim_model/grid.py).argfile,max_sim_steps,output_step_interval,print_on_stop=>0) to BNG2.pl underauto, and refuses them by name underbngsim._Cum).tests/test_am_trajectory_review_990.py,tests/test_bngsim_output_grid_review.py).sample_timesdifference. The bridge integrates from the first sample time and ignores an earliert_end. That is On the default bngsim backend, a BNGL simulate() whose sample_times start after t_start is integrated from the first listed time, so the initial state is reported there and every scored point is shifted #872, which predates this PR. It is stated in the docs and pinned by a strict xfail.