Ken Wesson <[email protected]> writes:

> On Sat, Jan 22, 2011 at 11:26 AM, Eric Schulte <[email protected]> wrote:
>> Nice concise example,
>
> Thanks.
>
>> A while back I implemented something similar; a propagator system using
>> agents fed with `send', coming in at a slightly more verbose ~35 lines
>> of code.
>>
>> http://cs.unm.edu/~eschulte/research/propagator/
>
> Interesting. But I have some questions ...
>
>> (defn set-cell "Set the value of a cell" [cell value]
>>   (send cell (fn [_] value)))
>
> Why (fn [_] value) instead of (constantly value)? OK, actually
> (constantly value) is textually longer, but I'd argue it might be
> clearer. And it works; (constantly foo) accepts all arities. It's
> something like (fn [& _] foo).
>

I agree constantly would have been a better choice, had I known it
existed.

>
> A question for Clojure's developers: why does restart-agent require
> the agent to be in a failed state? That seems to be a superfluous
> requirement. Atoms have reset! and refs ref-set to clobber the old
> value; agents would have something analogous if restart-agent worked
> on agents that weren't failed too.
>
>> (defmacro propagator "Define a new propagator."
>>   [name in-cells out-cells & body]
>>   `(do
>>      (defn ~(with-meta name
>>               (assoc (meta name)
>>                 :in-cells in-cells :out-cells out-cells))
>
> Whoa. Why not
>
> (defmacro propagator "Define a new propagator."
>   [name in-cells out-cells & body]
>   `(do
>      (defn ~(vary-meta name assoc
>               :in-cells in-cells :out-cells out-cells)
>
> ?
>

Again, I work with the functions I am aware of.  Thanks for these
pointers, they are very helpful especially as it can be difficult to
learn new functions once a sufficient subset has been discovered.

>
>>      (doseq [cell# ~in-cells] (add-neighbor cell# ~name))
>
> There's no forward-declaration of add-neighbor before this.
>
>>      ~name))
>
> Usually a deffoo form returns the new var, not just a symbol. This can
> be fixed with
>
> (declare add-neighbor)
>
> (defmacro propagator "Define a new propagator."
>   [name in-cells out-cells & body]
>   `(let [v# (defn ~(vary-meta name assoc
>                      :in-cells in-cells :out-cells out-cells)
>               ~in-cells ~@body)
>      (doseq [cell# ~in-cells] (add-neighbor cell# ~name))
>      v#))
>
> which captures the return value of the defn and returns it after the doseq.
>

Agreed, that is an improvement.

>
>> (defmacro run-propagator
>>   "Run a propagator, first collect the most recent values from all
>> cells associated with the propagator, then evaluate."
>>   [propagator]
>>   `(let [results# (apply ~propagator (map deref (:in-cells ^#'~propagator)))]
>>      (doseq [cell# (:out-cells ^#'~propagator)]
>>        (when (not (= @cell# results#))
>>          (send cell# (fn [_#] results#))))
>>      results#))
>
> Why not use your already-defined set-cell function on that 
> second-to-last-line?
>
> Also your syntax for accessing the metadata seems a bit odd. Why not
> (meta ~propagator)?
>

Agreed (meta ~propagator) is much more clear.

>
> Good use of metadata, though.
>
>> (defmacro add-neighbor "Add a neighbor to the given cell."
>>   [cell neighbor]
>>   `(add-watcher
>>     ~cell :send
>>     (agent nil :validator (fn [_#] (do (future (run-propagator ~neighbor)) 
>> true)))
>>     (fn [_# _#])))
>
> What is "add-watcher"? I'm somewhat familiar with add-watch, whose
> syntax would be somewhat different. There'd be nothing between the
> watcher key :send and the (fn ...), and that function would take four
> arguments.
>

This was an old project (last spring) and it's version of clojure
(org.clojure/clojure "1.1.0-alpha-SNAPSHOT") is not even resolved any
longer by lein.  It appears that the add-watcher function has been
deprecated and replaced by add-watch, which doesn't require that the
function return true allowing the simpler construction you suggest
below.

>
> I'd have probably used something like:
>
> (defmacro add-neighbor "Add a neighbor to the given cell."
>   [cell neighbor]
>   `(add-watch
>     ~cell :send
>     (fn [_# _# _# _#]
>       (future (run-propagator ~neighbor))))
>
> You could also get rid of the need to forward-declare add-neighbor by
> moving the propagator macro last.
>
> Convention also is that you'd call cell defcell and propagator
> defpropagator, since they expand to def forms.
>

Ah, this is a big oversight on my part.  Thanks for mentioning.

I've implemented your suggestions, and committed the results to
http://gitweb.adaptive.cs.unm.edu/propagator.git

>
> Now some more general notes:
>
> Having multiple out cells is interesting, but maybe redundant. Even
> with only a single out cell, the same effect can be had if you have an
> in cell, a bunch of out cells, and for each out cell a propagator that
> just returns the in cell's value.
>
> At that point the distinction between cells and propagators can go
> away: every cell has a propagator that sets its own value if any of
> its in-cells change. A cell that's intended to be an input can have an
> empty set of in-cells, which will result in it never doing anything
> since it will never be triggered.
>

Interesting point.  I think your suggestion would lead to simpler code,
but IMO the existing design is simpler conceptually, I like the divide
of propagators as functions and cells as data, rather than every
function having an associated piece of data (namely its output).

I'm not sure which implementation would lead to a more efficient running
system.  Having fewer larger propagators which can set the values of
many cells at once, or more smaller propagators each of which only sets
the value for a single cell.

>
> Of course the effect of that is to turn it into an almost-spreadsheet.
> Giving the cells a spatial organization is the last step to making it
> a spreadsheet. :)
>

That would be a great application for this system.  Each cell of the
spreadsheet could be a cell, and each formula could be a propagator.
I've implemented this and it seems to work well, I've committed this to
the repo above, and posted the spreadsheet code with example usage into
a gist at https://gist.github.com/792968

>
> I hope you don't think all of that was too critical. It's very interesting 
> work.

Not at all, I appreciate the code review, your function suggestions and
convention reminders were very useful.

Many Thanks -- Eric

-- 
You received this message because you are subscribed to the Google
Groups "Clojure" group.
To post to this group, send email to [email protected]
Note that posts from new members are moderated - please be patient with your 
first post.
To unsubscribe from this group, send email to
[email protected]
For more options, visit this group at
http://groups.google.com/group/clojure?hl=en

Reply via email to