Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rule[e] collisions #19

Open
hmedina opened this issue Jun 1, 2023 · 6 comments
Open

Rule[e] collisions #19

hmedina opened this issue Jun 1, 2023 · 6 comments

Comments

@hmedina
Copy link
Collaborator

hmedina commented Jun 1, 2023

When rules use names like _pert_ or _init_, which are valid KaSim identifiers, it becomes ambiguous if an entry in KaTie's output is due to a special non-event step, or an event from one of those rules.

For a concrete example, see the test in the (just added) branch https://github.com/jonathan-laurent/KaTie/tree/rule_special_string_collision

Model:

%agent: timer(s{tik,tok})
timer(s{tik}) <-> timer(s{tok}) @ 1, 1

%agent: mischief()

%init: 1 mischief()
%mod: [E] = 1 do $ADD 1 mischief() ; repeat [false]

'_pert_' mischief()+ @ 1
'_init_' mischief()+ @ 1

%mod: |mischief()| = 4 do $STOP ;

Query:

query 'rule_name_collision.csv'
match e:{ +m: mischief() }
return {'rule': rule[e]}

KaTie output:

'rule'
"_init_"
"_pert_"
"_pert_"
"_init_"

It is unclear of some KaSim bug triggered a second round of init steps. It is also unclear if the 2nd or 3rd rows are due to a rule application or a perturbation.

Checking the trace-summary.json:

{
  "0": [ "_init_", "new(mischief.0)" ],
  "1": [ "_pert_", "new(mischief.1)" ],
  "2": [ "_pert_", "new(mischief.2)" ],
  "3": [ "_init_", "new(mischief.3)" ]
}

The summary does not have the information to clarify what happened.

The long summary, trace-summary-long.json is a tad more informative, showing that the second init is likely a rule, since it contains a tests block:

{
  "0": [ "_init_", "new(mischief.0)" ],
  "1": [ "_pert_", { "actions": "new(mischief.1)", "tests": "" } ],
  "2": [ "_pert_", { "actions": "new(mischief.2)", "tests": "" } ],
  "3": [ "_init_", { "actions": "new(mischief.3)", "tests": "" } ]
}

Diving into a (prettier-formated) raw trace shows:

"trace":[
    [3,[[0,[0,1],[]]]],
    [1,2,[[],[[0,[1,1],[]]],[],[],[]],[-1,0.8967991814821501,1,null]],
    [2,"$APPLY 1 mischief()+",[[],[[0,[2,1],[]]],[],[],[]],[-1,0.8967991814821501,1,null]],
    [1,3,[[],[[0,[3,1],[]]],[],[],[]],[-1,1.1282034450621492,2,null]]]

Which reveals the steps were of types init, rule, pert, rule.

It seems to me that asking the "rule name" of an occurrence that's not an event, is ill-defined and should return a NULL. Besides that, I would suggest adding a measure that returns the step type (i.e. one of the contents of the json's /dict/step array of ["Subs","Rule","Pert","Init","Obs","Dummy"]). This would allow type-safe filtering of initial conditions.

@jonathan-laurent
Copy link
Owner

jonathan-laurent commented Jun 2, 2023

You are making a good case here that there should be separate rule[e] and event_kind[e] measures. This would add some conceptual clarity and avoid the clashes you mentioned. On the other hand, it would make some stuff a lot more verbose. In practice, it is often pretty useful to consider init events as special applications of an _init_ rule and have the ability to quickly getting a small string label for the step. Maybe we can add a label[e] measure that returns strings like init, rule:my_rule_name, pert...

In any case, there is still a tradeoff, which in this case is to replace one thing that is not exactly right but useful and what you want in most cases by 3 different things. This remembers me the "Worst is Better" debate.

@hmedina
Copy link
Collaborator Author

hmedina commented Jun 2, 2023

This "Worse is Better"? The author's response in Is Worse Really Better? ends with an interesting paragraph (ref. 6 on that Wikipedia article).

If you feel this is an area where correctness can be compromised in favor of simplicity, this GH issue will document this behavior, so we can close this and move on.

@jonathan-laurent
Copy link
Owner

Yes, this is the correct reference.

Note that this behavior is already documented, although the potential for name clashes could be emphasized:

rule[e]: string: indicates the name of the rule underlying event e. Special values '_init_', '_pert_' and '_obs_' are returned for initial events, perturbation events and observation events respectively.

What's your opinion here? Would you rather have the three measures I mentioned earlier? Note that this is not strictly speaking a correctness issue. The current behavior is well documented and the implementation does what it promises. The only thing that could be argued is that it is error-inducing.

@hmedina
Copy link
Collaborator Author

hmedina commented Jun 2, 2023

My opinion is that an object's name should not be allowed to be confused for its type: "rule name" is only a property of rule-type steps. Outside of rule applications, the rule-name measure is undefined behavior, and should therefore return a null or some other marker of missing-value.

Consider a query:

match e: {+a: A() }
when rule[e] = '_init_'
return { ... }

Did the user mean to match only nameless steps of type 3 1, or named steps of type 1 ? If negated, did the user mean to disregard the initial conditions, or instances of the rule _init_, or both? Before running KaSim, modelers will have to have read the KaTie documentation, so they can make their models avoiding the strings reserved as special here, if at a later time they want to run KaTie on their traces.

If the desire for overloading rule is strong, I think just one measures (step_type) is sufficient:

  • step_type[e] -> str, indexes the trace's step defining list1 with the very first integer of the step object, and returns the type of this step. The corresponding string is human readable, but just returning the integer would be sufficient.

This would allow the unambiguous query:

match e: {+a: A() }
when not ( step_type[e] = 'init')
return { ... }

Of course, since "event" means one thing in KaTools-land and another in KaTie-land2, I'm suggesting naming with step instead of event, but event_type could also provide this functionality.

Footnotes

  1. Step type defined in the trace's /dict/step/ array 2

  2. https://github.com/Kappa-Dev/KappaTools/issues/664 ; KaSim's "events" are rule applications; KaTie's "events" include rule applications, plus all other types of trace steps

@jonathan-laurent
Copy link
Owner

This makes sense. Another point of view is that a trace is morally a sequence of rule applications and init/pert events are instances of some special rules. Special names such as @init and @pert could be used instead to avoid name clashes (since the @ character cannot belong to a standard rule name). To be honest, I personally favor your solution but the alternative I suggested may be favored by some schools of thoughts that value API-minimalism.

Regarding using the name step, I disagree since KaTie consistently uses the term event for this. Also, it is unclear to me what KaSim currently defines as an event (as was shown by the debate of the meaning of [E]). Ultimately, there should only be one notion of an event and one notion of an agent id that is shared across all tools.

@hmedina
Copy link
Collaborator Author

hmedina commented Jun 3, 2023

Using special strings for special cases is a fair approach; since KaSim currently does not allow those as rule names, nor will it generate similar as derived rule names (e.g. r -> r_op), nor do those make sense as the short descriptions KaTie generates for un-named rules, those strings unambiguously mark if the matched step is of those types.

The "ignore steps that are not rule applications" becomes:

match e: {+a: A() }
when not ( rule[e] = '@init') and not ( rule[e] = '@pert')
return { ... }

'@obs' I'm not catching because it can't trigger an agent creation, so it can't match e (right?)

I agree that having "event" mean different things in different tools is detrimental to new users trying to approach the ecosystem. That is a larger discussion we will have to revisit if & when KaTie is merged into the KaTools repo.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants