{"action":"created","issue":{"url":"https://api.github.com/repos/rbrush/clara-rules/issues/86","labels_url":"https://api.github.com/repos/rbrush/clara-rules/issues/86/labels{/name}","comments_url":"https://api.github.com/repos/rbrush/clara-rules/issues/86/comments","events_url":"https://api.github.com/repos/rbrush/clara-rules/issues/86/events","html_url":"https://github.com/rbrush/clara-rules/issues/86","id":56965897,"number":86,"title":"Accumulate nodes with and without :retract-fn given","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":null,"comments":2,"created_at":"2015-02-08T20:20:21Z","updated_at":"2015-02-09T13:07:34Z","closed_at":null,"body":"I have been looking into the AccumulateNode of clara.rules.engine a bit recently to fully understand its semantics.\r\nThe one I want to ask about here centers on the :retract-fn given or not given in an Accumulator for Clara.\r\n\r\nIf everything works here as expected, then there is no real \"issue\".  If not, then some work could come out of this.\r\n\r\nThis is easiest to just demonstrate with a test case, mostly ripped from an existing clara.test-rules test.  I modified it to demonstrate my point.\r\n\r\n```clj\r\n(deftest test-accumulate-with-retract\r\n  (let [collect-with-retract (accumulate\r\n                              :initial-value []\r\n                              :reduce-fn conj\r\n                              :combine-fn concat\r\n\r\n                              ;; Retract by removing the retracted item.\r\n                              ;; In general, this would need to remove\r\n                              ;; only the first matching item to achieve expected semantics.\r\n                              :retract-fn (fn [reduced item]\r\n                                            (remove #{item} reduced))\r\n\r\n                              ;; Sort here and return the smallest.\r\n                              :convert-return-fn (fn [reduced]\r\n                                                   (first\r\n                                                    (sort #(< (:temperature %1) (:temperature %2))\r\n                                                          reduced))))\r\n\r\n        collect-without-retract (assoc collect-with-retract\r\n                                  ;; Note: Using the - Retractions do nothing by default for `clara.rules/accumulate`.\r\n                                  :retract-fn (fn [reduced retracted] reduced))\r\n                                       \r\n        coldest-query (dsl/parse-query [] [[?t <- collect-with-retract :from (Temperature (< temperature 20))]])\r\n        \r\n        results-with-retract-fn (-> (mk-session [coldest-query])\r\n                                    (insert (->Temperature 10 \"MCI\"))\r\n                                    (insert (->Temperature 17 \"MCI\"))\r\n                                    (insert (->Temperature 15 \"MCI\"))\r\n                                    (insert (->Temperature 80 \"MCI\"))\r\n                                    (retract (->Temperature 10 \"MCI\"))\r\n                                    (query coldest-query)\r\n                                    set)\r\n\r\n        coldest-query (dsl/parse-query [] [[?t <- collect-without-retract :from (Temperature (< temperature 20))]])\r\n\r\n        results-without-retract-fn (-> (mk-session [coldest-query])\r\n                                       (insert (->Temperature 10 \"MCI\"))\r\n                                       (insert (->Temperature 17 \"MCI\"))\r\n                                       (insert (->Temperature 15 \"MCI\"))\r\n                                       (insert (->Temperature 80 \"MCI\"))\r\n                                       (retract (->Temperature 10 \"MCI\"))\r\n                                       (query coldest-query)\r\n                                        set)]\r\n\r\n    ;; The accumulator result should be\r\n    (is (= #{{:?t (->Temperature 15 \"MCI\")}}\r\n           results-with-retract-fn))\r\n\r\n    (is (= #{{:?t (->Temperature 15 \"MCI\")}}\r\n           results-without-retract-fn))))\r\n\r\n```\r\n\r\nThe results of this were, as I expected - a failure:\r\n\r\n```clj\r\nFAIL in (test-accumulate-with-retract) (test_rules.clj:398)\r\nexpected: (= #{{:?t (->Temperature 15 \"MCI\")}} results-without-retract-fn)\r\n  actual: (not (= #{{:?t #clara.rules.testfacts.Temperature{:temperature 15, :location \"MCI\"}}} #{{:?t #clara.rules.testfacts.Temperature{:temperature 10, :location \"MCI\"}}}))\r\n```\r\n\r\nAn accumulator with no provided :retract-fn does nothing upon retract by default.  This has the effect (from some perspectives at least) of potentially preserving \"old\" working state\r\nwith respect to the TMS.\r\n\r\nThe workaround for this, is obviously to specify a :retract-fn for your accumulator when you want to be able to \"back out\" propagations that are based on no-retracted facts.\r\n\r\nI went in to \"fix\" this issue and implement a check in clara.rules.engine to use the :retract-fn if it is given, otherwise fall back to reaccumulating the state of working memory facts\r\nwith the retracted items removed.  This would require no :retract-fn to be given by default on the `clara.rules/accumulate` impl of course.\r\n\r\nWhen I went to pursue this direction in the engine, I found that the AccumulateNode actually does not retain any memory of elements that it has previously been matched on.  So the ability to \"re-accumulate the world\" when no :retract-fn is given does not seem possible under the current design.\r\n\r\nOnce I thought about this a minute, I was led to think this may by design.  The current AccumulateNode impl allows for accumulators to \"throw\" away facts as they are accumulated in a\r\nstreaming sort of fashion.  I may have read or heard you mention something to this effect at some point in time.\r\n\r\nI am mostly wanting to clarify that my thoughts on this are correct, that this is by design.\r\nIf this design decision has real utility, like the ability to release memory to already accumulated facts, etc. I'd be curious to hear some thoughts you have on that.\r\n\r\n---\r\n\r\nContinuing down this track.\r\n\r\nI'll note that I do not think that AccumulateWithJoinFilterNode has these same properties as AccumulateNode with regards to the :retract-fn semantics.  AccumulateWithJoinFilterNode maintains\r\nall candidate facts to be accumulated in its node memory.  Upon right-retract element retraction, the entire state of the candidate facts are re-accumulated to produce a new value.  This impl\r\ndoes not even consider the usage of the :retract-fn.  An aside to that point, I'm skeptical that it could still get some benefit from if (see [1]).\r\nSo my observation is that AccumulateWithJoinFilterNode does not operate in the same \"streaming\" way that AccumulateNodes do.\r\n\r\n\r\n---\r\n\r\nMy main purpose of this post was for my own understanding of these semantics.  I will note that I think it may be a bit more difficult to implement effective accumulators\r\nfor some workflows using AccumulateNode where I want a logically correct result value with respect to current (or \"final\") working memory.\r\n\r\nI'll demonstrate with yet another test case that I find straight-forward:\r\n\r\n```clj\r\n\r\n(let [min-temp (dsl/parse-query [] [[?t <- (acc/min :temperature) :from [Temperature]]])\r\n\r\n      session (-> (mk-session [min-temp])\r\n                  (insert (->Temperature 1 \"MCI\"))\r\n                  (insert (->Temperature 2 \"LAX\"))\r\n                  (retract (->Temperature 1 \"MCI\"))\r\n                  fire-rules)]\r\n\r\n  (is (= #{{:?t 2}}\r\n         (set (query session min-temp)))))\r\n\r\n\r\n;; Results in\r\n\r\nFAIL in <some-ns-here>\r\nexpected: (= #{{:?t 2}} (set (query session min-temp)))\r\n  actual: (not (= #{{:?t 2}} #{{:?t 1}}))\r\nfalse\r\n\r\n```\r\n\r\nThis is using the min accumulator from clara.rules.accumulators namespace.  If I wanted an accumulator that could return the min Temperature *remaining* in working memory\r\nat the end of changes, I'd need to maintain state of all previously seen facts via the reduced values and implement a :retract-fn that worked.  I don't think any previous facts could be ignored, since it is arbitrary which facts are removed at a later time.\r\n\r\nAn impl for this looks something like:\r\n```clj\r\n\r\n(defn min\r\n  \"Returns an accumulator that returns the minimum value of a given field.\"\r\n  [field & {:keys [returns-fact]}]\r\n  (let [reduce-fn (fn [[seen value] item]\r\n                    (if (or (nil? value)\r\n                            (< (field item) (field value)))\r\n\r\n                      [(conj (or seen []) item) item]\r\n\r\n                      [(conj (or seen []) item) value]))]\r\n    (accumulate\r\n\r\n     :initial-value nil\r\n\r\n     :reduce-fn reduce-fn\r\n\r\n     :combine-fn (fn [[seen1 value1] [seen2 value2]]\r\n                   (if (or (nil? value1)\r\n                           (< (field value2)\r\n                              (field value1)))\r\n\r\n                     [(into (or seen1 []) seen2) value2]\r\n\r\n                     [(into (or seen1 []) seen2) value1]))\r\n     \r\n     :retract-fn (fn [[seen _] item]\r\n                   (reduce reduce-fn nil (remove #(= item %) seen)))\r\n\r\n     :convert-return-fn (if returns-fact\r\n                          second\r\n                          (comp field second)))))\r\n```\r\n\r\nThis doesn't seem like something necessarily trivial to implement.  The reason being is that it is playing the role of managing the \"node memory\" for accumulator.\r\nPerhaps this tradeoff is acceptable since it can still be written to work this way if the default design is not sufficient.\r\n\r\nI think it is less obvious that AccumulateNode and AccumulateWithJoinFilterNode will behave differently though.\r\n\r\nThanks for the feedback!  Sorry it this was long-winded.\r\n\r\n---\r\n\r\n\r\n[1]\r\n\r\n(note this may not be worth the trouble, just a thought)\r\n\r\n1) accumulated results were stored in additional node memory (is this too much overhead?)\r\n2) the filtering of candidates was done first\r\n3) the retracted facts were retrieved from that filtered candidates\r\n4) the accumulator had the :retract-fn ran in a `reduce` given previously-accumulated-result + retracted-facts\r\n  \r\n"},"comment":{"url":"https://api.github.com/repos/rbrush/clara-rules/issues/comments/73505355","html_url":"https://github.com/rbrush/clara-rules/issues/86#issuecomment-73505355","issue_url":"https://api.github.com/repos/rbrush/clara-rules/issues/86","id":73505355,"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},"created_at":"2015-02-09T13:07:34Z","updated_at":"2015-02-09T13:07:34Z","body":"Thanks for the details on this.  I can see how there could be utility in leaving the AccumulateNode open for the streaming use-case.\r\n\r\nI will agree that it would be helpful for some built-in Clara accumulators to offer possibly both of these behaviors.  That is a good point that some helper functions may be nice.  It probably wouldn't be too challenging to take an accumulator and decorate it to hold \"node memory\" and to perform a naiive `:retract-fn` that just re-accumulates based on the new state of memory.\r\nOf course a better `:retract-fn` could be written in some cases depending on the operation of the accumulator.  However, that would be left up to the writer at that point."}}