search

dollabs / pamela

Probabalistic Advanced Modeling and Execution Learning Architecture
Apache License 2.0
233 stars 13 forks source link

Improve unparser #129

Closed tmarble closed 7 years ago

tmarble commented 7 years ago

@dcerys all of your concerns should be addressed in the recent push of this branch.

NOTE: sort-map2 is truly intended as a stop-gap measure until sort-map can be properly updated in plan-schema. I have a fix in mind for this, but must separate out bumping the plan-schema dependency from this PR.

pmdoll commented 7 years ago

Doc notes are incorrect and state the inverse of the function.

(defn unparse-model
+  "Load model(s) and build intermediate representation (IR)"
pmdoll commented 7 years ago

sort-map in plan-schema is very clear and concise. What is the motivating example that requires everything else but symbols to be sorted?

tmarble commented 7 years ago

@pmdoll that sort-map is clear and concise is not helpful when it exposes the underlying bug that by using sorted-map it cannot handle maps of heterogeneous key types. This has been discussed as a problem with sorted-map (e.g. CLJ-1983).

The motivation for having sorted maps is that we often express Pamela related artifacts as maps: the IR, the HTN and TPN. Having these maps in sorted order makes it easier to visually compare them and often makes it possible to do a direct text comparison (i.e. using diff). However the IR, in particular, contains maps with heterogeneous keys and thus sort-map (in it's current form) will fail.

The next update to this branch will include a corrected version of sort-map (called sort-map2 here until the actually implementation has been placed in plan-schema).