Fix rules engine treating behavior/signal synonyms as separate entries - #423
Open
heberlr wants to merge 1 commit into
Open
Fix rules engine treating behavior/signal synonyms as separate entries#423heberlr wants to merge 1 commit into
heberlr wants to merge 1 commit into
Conversation
Hypothesis_Ruleset::add_behavior/find_behavior and Hypothesis_Rule::add_signal/ find_signal keyed their maps by the raw string from the rules file instead of the canonical, synonym-resolved name, even though find_behavior_index()/ find_signal_index() were already computing the canonical index just to validate it. Two rules for the same behavior spelled with different synonyms (e.g. "cycle entry" vs "exit from cycle phase 0") produced two separate Hypothesis_Rule objects instead of one merged multivariate Hill response, so whichever rule was applied last silently overwrote the other's contribution. Canonicalize behavior/signal names before they're used as map keys or compared by name, in add_behavior, find_behavior, add_signal, find_signal, set_half_max, set_hill_power, and set_response. Also fix signals_map[signal] = signals_map.size(), which used map size as a proxy for vector index and could collide when the same signal was added twice with opposite responses in one rule; and drop a dead code path in add_rule that would have fought the new canonicalization. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Collaborator
Author
|
Attached is the template model to demonstrate the bug. |
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.
Summary
(e.g.
cycle entryvsexit from cycle phase 0) were being stored as twoseparate
Hypothesis_Ruleentries instead of one merged multivariate Hillresponse, because the rules engine keyed its internal maps by the raw
string from the CSV/XML rule instead of the canonical, synonym-resolved
name. Whichever rule applied last at runtime silently overwrote the other's
contribution to the phenotype parameter.
or compared by name (
add_behavior,find_behavior,add_signal,find_signal,set_half_max,set_hill_power,set_response).signals_map[signal] = signals_map.size()used map size as a stand-in for vector index; this diverges (and can
collide with another signal's index) when the same signal is added twice
with opposite responses in one rule.
add_rulethat reassigned a rule's canonicalbehaviorfield back to the caller's raw synonym string, which would haveundone the canonicalization above.
evaluated independently and summed into the Hill function, not merged into
a shared per-name structure), but did share the raw-string lookup, which is
now fixed consistently for both.
How test it
See the attached example model in the comments below (template project, using
two rules for the same behavior via different synonyms —
pressure decreases cycle entryandsubstrate increases exit from phase 0) demonstrating thebug before this fix and the corrected merged behavior after it.