{"action":"created","issue":{"url":"https://api.github.com/repos/rbrush/clara-rules/issues/132","labels_url":"https://api.github.com/repos/rbrush/clara-rules/issues/132/labels{/name}","comments_url":"https://api.github.com/repos/rbrush/clara-rules/issues/132/comments","events_url":"https://api.github.com/repos/rbrush/clara-rules/issues/132/events","html_url":"https://github.com/rbrush/clara-rules/issues/132","id":111893460,"number":132,"title":"Relaxing restriction on how accum :result-binding can be used","user":{"login":"mrrodriguez","id":3483575,"avatar_url":"https://avatars.githubusercontent.com/u/3483575?v=3","gravatar_id":"","url":"https://api.github.com/users/mrrodriguez","html_url":"https://github.com/mrrodriguez","followers_url":"https://api.github.com/users/mrrodriguez/followers","following_url":"https://api.github.com/users/mrrodriguez/following{/other_user}","gists_url":"https://api.github.com/users/mrrodriguez/gists{/gist_id}","starred_url":"https://api.github.com/users/mrrodriguez/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/mrrodriguez/subscriptions","organizations_url":"https://api.github.com/users/mrrodriguez/orgs","repos_url":"https://api.github.com/users/mrrodriguez/repos","events_url":"https://api.github.com/users/mrrodriguez/events{/privacy}","received_events_url":"https://api.github.com/users/mrrodriguez/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"milestone":{"url":"https://api.github.com/repos/rbrush/clara-rules/milestones/14","html_url":"https://github.com/rbrush/clara-rules/milestones/0.9.0","labels_url":"https://api.github.com/repos/rbrush/clara-rules/milestones/14/labels","id":927200,"number":14,"title":"0.9.0","description":null,"creator":{"login":"rbrush","id":574824,"avatar_url":"https://avatars.githubusercontent.com/u/574824?v=3","gravatar_id":"","url":"https://api.github.com/users/rbrush","html_url":"https://github.com/rbrush","followers_url":"https://api.github.com/users/rbrush/followers","following_url":"https://api.github.com/users/rbrush/following{/other_user}","gists_url":"https://api.github.com/users/rbrush/gists{/gist_id}","starred_url":"https://api.github.com/users/rbrush/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/rbrush/subscriptions","organizations_url":"https://api.github.com/users/rbrush/orgs","repos_url":"https://api.github.com/users/rbrush/repos","events_url":"https://api.github.com/users/rbrush/events{/privacy}","received_events_url":"https://api.github.com/users/rbrush/received_events","type":"User","site_admin":false},"open_issues":1,"closed_issues":5,"state":"open","created_at":"2015-01-08T15:29:37Z","updated_at":"2015-10-18T14:38:36Z","due_on":null,"closed_at":null},"comments":5,"created_at":"2015-10-16T19:11:56Z","updated_at":"2015-10-21T21:54:34Z","closed_at":null,"body":"\r\nI see the change from\r\nhttps://github.com/rbrush/clara-rules/commit/ca68b9dc140dcb088c5236eb22d2cb6be5d4b5ac\r\nis meant to prevent `:result-binding`s from being used my subsequent conditions within a rule LHS.\r\n\r\nI'm not positive on the reason behind this, so I'd certainly like your feedback on it.  However, I am thinking that the reason for this is that Clara will re-order the conditions in a rule so that the order of conditions is: simple conditions -> negation conditions -> accumulator conditions -> test conditions.  I see this in the clara.rules.compiler/condition-comp function.\r\n\r\nThis restriction is interestingly not always applicable to simple conditions if the binding is not a direct field equality unification.\r\n\r\nI this is best just demonstrated with an example.\r\nNote: This example is obviously contrived to just make it as short as I could easily demonstrate.  I realize it doesn't make a lot of sense to do it.\r\n\r\nThis works:\r\n ```clj\r\n(let [r (dsl/parse-rule [[?t <- (acc/max :temperature :returns-fact true) :from [Temperature]]\r\n                         [Temperature (= (identity ?t) this)]]\r\n\r\n                        (println \"?t\" ?t))\r\n      s (mk-session [r])]\r\n  (-> s\r\n      (insert (->Temperature 1 \"MCI\"))\r\n      fire-rules))\r\n\r\n?t #clara.rules.testfacts.Temperature{:temperature 1, :location MCI}\r\n;= #<LocalSession clara.rules.engine.LocalSession@764670aa>\r\n \r\n```\r\n\r\nThis does not work:\r\n ```clj\r\n(let [r (dsl/parse-rule [[?t <- (acc/max :temperature :returns-fact true) :from [Temperature]]\r\n                         [Temperature (= ?t this)]]\r\n\r\n                        (println \"?t\" ?t))\r\n      s (mk-session [r])]\r\n  (-> s\r\n      (insert (->Temperature 1 \"MCI\"))\r\n      fire-rules))\r\n\r\nExceptionInfo Using accumulator result bindings in other expressions in a rule left-hand side is currently not supported. Binding: :?t in production {:lhs [{:result-binding :?t, :accumulator (clara.rules.accumulators/max :temperature :returns-fact true), :from {:type clara.rules.testfacts.Temperature, :constraints []}} {:type clara.rules.testfacts.Temperature, :constraints [(= ?t this)]}], :rhs (println \"?t\" ?t)}  clojure.core/ex-info (core.clj:4403)\r\n \r\n```\r\n\r\nThe reason (I believe) is that the difference in the condition constraint `(= ?t this)` vs `(= (identity ?t) this)`.\r\nWhen an seq-style expression is involved in the binding unification, the operation is \"deferred\" into a later :test condition.  With this, I believe some of the efficiency of the hash-based join may be lost, but I am not positive there.\r\n\r\n--\r\n\r\nIn the case of negation conditions, I cannot find a way where I can successfully use a `:result-binding` from an accumulator at all.  However, the error message is for negation conditions hitting thi srestriction are more mysterious than the errors for simple conditions.\r\n\r\n```clj\r\n(let [r (dsl/parse-rule [[?t <- (acc/max :temperature :returns-fact true) :from [Temperature]]\r\n                         [:not [Temperature (= (identity ?t) this)]]]\r\n\r\n                        (println \"?t\" ?t))\r\n      s (mk-session [r])]\r\n  (-> s\r\n      (insert (->Temperature 1 \"MCI\"))\r\n      fire-rules))\r\n\r\nExceptionInfo Using variable that is not previously bound. This can happen when an expression uses a previously unbound variable, or if a variable is referenced in a nested part of a parent expression, such as (or (= ?my-expression my-field) ...). \r\nUnbound variables: #{?t}  clojure.core/ex-info (core.clj:4403)\r\n\r\n;;; As well as\r\n\r\n(let [r (dsl/parse-rule [[?t <- (acc/max :temperature :returns-fact true) :from [Temperature]]\r\n                         [:not [Temperature (= ?t this)]]]\r\n\r\n                        (println \"?t\" ?t))\r\n      s (mk-session [r])]\r\n  (-> s\r\n      (insert (->Temperature 1 \"MCI\"))\r\n      fire-rules))\r\n\r\nExceptionInfo Using accumulator result bindings in other expressions in a rule left-hand side is currently not supported. Binding: :?t in production {:lhs [{:result-binding :?t, :accumulator (clara.rules.accumulators/max :temperature :returns-fact true), :from {:type clara.rules.testfacts.Temperature, :constraints []}} [:not {:type clara.rules.testfacts.Temperature, :constraints [(= ?t this)]}]], :rhs (println \"?t\" ?t)}  clojure.core/ex-info (core.clj:4403)\r\n\r\n```\r\n\r\nI haven't went into the details yet of why negation conditions are not behaving the same as the simple conditions here.\r\n\r\n---\r\n\r\nI think this behavior is somewhat confusing and the limitation has caused me some difficulty in rules that we do typically want to write.  I realize it can be worked around by separating the accumulated `:result-binding` rule from the rest of the logic that needs to reason about it.  This can be done via an intermediate fact to relay the accumulated result information to further conditions in another rule.\r\n\r\nI'm curious if this restriction could be reconsidered and removed by moving the unification to a :test condition when a simple condition or negation condition wished to refer to the `:result-binding` of an accumulator.  I think this may be straight forward for simple conditions.  I'm not sure on negation conditions as I've mentioned above.\r\n\r\nThanks for the feedback on this!\r\n"},"comment":{"url":"https://api.github.com/repos/rbrush/clara-rules/issues/comments/150036036","html_url":"https://github.com/rbrush/clara-rules/issues/132#issuecomment-150036036","issue_url":"https://api.github.com/repos/rbrush/clara-rules/issues/132","id":150036036,"user":{"login":"rbrush","id":574824,"avatar_url":"https://avatars.githubusercontent.com/u/574824?v=3","gravatar_id":"","url":"https://api.github.com/users/rbrush","html_url":"https://github.com/rbrush","followers_url":"https://api.github.com/users/rbrush/followers","following_url":"https://api.github.com/users/rbrush/following{/other_user}","gists_url":"https://api.github.com/users/rbrush/gists{/gist_id}","starred_url":"https://api.github.com/users/rbrush/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/rbrush/subscriptions","organizations_url":"https://api.github.com/users/rbrush/orgs","repos_url":"https://api.github.com/users/rbrush/repos","events_url":"https://api.github.com/users/rbrush/events{/privacy}","received_events_url":"https://api.github.com/users/rbrush/received_events","type":"User","site_admin":false},"created_at":"2015-10-21T21:54:34Z","updated_at":"2015-10-21T21:54:34Z","body":"The \"previous-result\" variable could probably use a better name. That check isn't to compare it with the previous result of the accumulator, but with any other constraint that uses the same binding. For instance, a rule like this:\r\n\r\n```clj\r\n(defrule example\r\n   [?temp <- Temperature (= ?t temperature)]\r\n   [?t <- (acc/max :temperature) :from [Temperature]]\r\n  =>\r\n  (println ?temp \"is the maximum temperature fact!\"))\r\n```\r\n\r\n(This is contrived and a bad way to write this logic, of course.) \r\n\r\nNotice that ?t is bound in the first condition, and re-bound as the result of the accumulator. So we'd only want the rule to fire if the ?t bound above matches the ?t resulting from the accumulator...which is what that check does. This wasn't necessary when we disallowed the use of accumulator results in other expressions, but now that we support it we have to make sure they are consistent.\r\n\r\nIt's also worth noting that we can put this logic in the accumulator node because we sort accumulators to be after other nodes in the network."}}