Repository navigation
fix(observables): apply a Molecules count filter only where BNG2 does - #64
Merged
Merged
Conversation
A stoichiometric comparison on a Molecules observable was applied to the per-species EMBEDDING count, not a molecule count. On one species A() holding 10 molecules, `A()>5` compared 1 > 5, failed, and the observable contributed 0 -- so A()>5, A()>9, A()>10 and A()>99 all read 0 where the model plainly has 10 molecules of A. BNG2 2.9.3 is the oracle and it is unambiguous: Perl2/Observable.pm tests $patt->Quantifier only inside the `Type eq "Species"` branch (lines 278-292), while the Molecules branch (lines 241-256) accumulates raw match counts and never inspects it. So in BNG2 `Molecules Q A()>5` and `Molecules Q A()` are the same observable and the threshold is inert rather than an error. The fix gates the filter on the observable type, matching that. Measured against BNG2 on the same model (A()=10, B()=3), all six columns now agree exactly; before, BNG3 returned 0 for four of them. The Species path is untouched and already agreed with BNG2. Co-Authored-By: Claude Opus 4.8 (1M context) <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
1 participant
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.
Second commit of #59, cherry-picked alone so it merges cleanly against eight lanes in
OdeIntegrator.cpp.The defect
A species count filter was comparing the number of matched embeddings against the molecule count, so
A()>5on a species withA()=10returned 0.The fix — and why it is a rule change, not a number change
BNG2 2.9.3 honours a quantifier only in the
Speciesbranch (Perl2/Observable.pm:278-292), not in theMoleculesbranch (:241-256), where it lowers the filter away entirely — its.netgroups carry no threshold. So:A()(A=10)A()>0A()>5A()>9A()>10B()>2(B=3)The threshold is ignored in the Molecules branch because that is what the oracle does. That is the honest framing: a user writing
A()>5today gets the molecule count with the quantifier silently dropped, and after this change they still get the molecule count — matching BNG2 — rather than a count filtered by an unsupported criterion.Verification
ctest -j4andpython -m pytest tests/pythongreen on the rebased tree, withUNDER TESTpaths asserted inside a pytest session against this worktree. Failing-first demonstrated by reverting the source in place (nogit stash).Merge hazard
cpp/engine/OdeIntegrator.cppcarries eight declared lanes. Review against the function list rather thangit status, and the verification token for this change is the branch-point symbol with its expected count measured on this branch.correctnessverified this independently:A_gt5,A_gt9,A_gt10,B_gt2,B_gt3all now return the molecule count, and the landing matches BNG2 on the cases that are easy to get wrong, not just the one originally flagged.