feat: new IChoiceMap interface, implementations (2/n) #52

sritchie · 2023-11-14T19:57:55Z

This PR contains an implementation of the choicemap interface and data types found in Gen.jl. I stuck very closely to their behavior, sparsely implementing clojure.core methods where they were necessary or appropriate.

This PR follows #51 .

Also, a note for reviewers... the design here of using a "Choice" or "ValueChoiceMap" type follows GenJAX, as well as a design suggested by @georgematheos in these two PRs:

We actually did the latter in #23, but get-choices wasn't actually returning an IChoiceMap in all cases. This change fixes that, as George recognized.

This change from Gen.jl is blessed by @alexlew, Vikash and @femtomc.

codecov · 2023-11-14T20:01:28Z

Codecov Report

Attention: 46 lines in your changes are missing coverage. Please review.

Comparison is base (55aa92f) 68.58% compared to head (9a75b78) 71.88%.

Files	Patch %	Lines
src/gen/choicemap.cljc	81.45%	38 Missing and 8 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main      #52      +/-   ##
==========================================
+ Coverage   68.58%   71.88%   +3.30%     
==========================================
  Files          19       20       +1     
  Lines         748      996     +248     
  Branches       17       24       +7     
==========================================
+ Hits          513      716     +203     
- Misses        218      256      +38     
- Partials       17       24       +7

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

georgematheos

@sritchie this looks great!

I left a number of small comments, but the overall structure looks great to me.

I would not fully trust my review on the parts of this code which provide implementations of Clojure/Clojurescript interfaces for choicemaps, since I am not familiar with those interfaces.

What is going to happen to dynamic/choice_map.cljc after this file is added?

georgematheos · 2023-11-16T00:08:32Z

src/gen/choicemap.cljc

+  ([m] (-get-value m))
+  ([m k] (-get-value (get-submap m k))))
+
+;; ## Choice


Nice - I like the rebrand of ValueChoiceMap to Choice.

georgematheos · 2023-11-16T00:12:39Z

src/gen/choicemap.cljc

+  arr/IArray
+  (to-array [_] [v])
+  (-from-array [_ xs idx]
+    [1 (Choice. (nth xs idx))])


I am probably misunderstanding the syntax here - but is this creating a vector [1, Choice(...)]?
If so, why is 1 the first element?
(If this is a syntax misunderstanding, pointers to a search term where I can understand this syntax? Thanks!)

keep questions like this coming! also we should get a REPL up and pair soon.

This function wants a return value of a 2-element vector with

how many items you took off of the input array xs

the new value you created with those elements

As described here https://github.com/InferenceQL/gen.clj/blob/main/src/gen/array.cljc#L10

So for Choice, we take 1 thing off from xs at start-idx, and create the Choice instance in the second slot.

(And yes you're right that it's creating the vector [1, Choice(,,,,)]

georgematheos · 2023-11-16T00:14:35Z

src/gen/choicemap.cljc

+  (-from-array [_ xs idx]
+    [1 (Choice. (nth xs idx))])
+
+  #?@(:clj


Just FYI - from here to the defn of Empty is beyond my current Clojure knowledge to understand (so I have not reviewed this chunk).

It is surprisingly annoying in Clojure to extend the base interfaces!! these are flexed in the tests, and I tried to match the implementations in Gen.jl for things like getindex. I'll comment below on what these do

georgematheos · 2023-11-16T00:17:47Z

src/gen/choicemap.cljc

+
+(declare kv->choicemap)
+
+(deftype EmptyChoiceMap [m]


@sritchie why does this accept an argument? (My guess: is it impossible to define types with no data in side in Clojure?)

georgematheos · 2023-11-16T00:54:28Z

src/gen/choicemap.cljc

+
+(declare equiv choicemap)
+
+(deftype DynamicChoiceMap [m]


I wonder about the name DynamicChoiceMap here.

I think of the name DynamicChoiceMap in Gen.jl as stemming from the distinction with the StaticChoiceMap. Due to the way staging works in Gen.jl, the address shape of a StaticChoiceMap is known at compile time. Thus, methods for updating traces can be specialized to different StaticChoiceMap shapes at compile time. While the shape of a StaticChoiceMap is known "statically" (at compile time), the shape of a DynamicChoiceMap is "dynamic" (it can be modified at runtime). Thus, update methods which accept a DynamicChoiceMap cannot be specialized at compile-time to the shape of the update choicemap.

Is there going to be a similar distinction between dynamic choicemaps and static choicemaps in gen.clj?

Not totally sure what a better alternative name would be if not, but here are a few ideas... Maybe HierarchicalChoiceMap or DictChoiceMap or GeneralChoiceMap?

Another possibility is to rename
ChoiceMap -> Choices
DynamicChoiceMap -> ChoiceMap
VectorChoiceMap -> ChoiceVector
and keep Choice.
This sort of implies VectorChoiceMaps don't have hierarchical structure, though.

So maybe the best bet is just to change DynamicChoiceMap -> GeneralChoiceMap to denote that (unlike a VectorChoiceMap) there are no restrictions on the key type.

It also seems reasonable to me to use DynamicChoiceMap for now even if it isn't super well motivated, just so things read similarly to Gen.jl for now. We could then possibly do a pass reconsidering names like this across all the Gen variants after GenJAX and gen.clj are more feature complete.

I like these thoughts — I would recommend that we stay with the current name to match Gen.jl for now, and that if we make a shift we do it with Gen.jl too.

BUT there is one subtlety — there is both a DynamicChoiceMap (which is mutable, yeehaw) AND a DynamicDSLChoiceMap in Gen.jl. The latter is not mutable once it's returned by get_choices, but is a wrapper around the trie stored in the dynamic trace.

Dynamic is doing a lot of work in Gen :)

georgematheos · 2023-11-16T01:33:16Z

src/gen/choicemap.cljc

+  [v]
+  (if (choicemap? v)
+    (and (not (has-value? v))
+         (core/empty? v))


am I right to understand - based on my quick googling - that this recursive call to core/empty? calls count on v? (or does it call the empty function you defined on each of the types above?)

I think empty? actually calls seq and sees if it returns nil. The empty function WITHOUT the question mark returns an empty instance of the supplied type:

gen.choicemap-test> (empty {1 2 3 4}) {} gen.choicemap-test> (empty [1 2 3]) [] gen.choicemap-test> (empty '(1 2 3)) () gen.choicemap-test> (empty (choicemap/choicemap [1 2 3])) #gen/choicemap {}

georgematheos · 2023-11-16T01:34:50Z

test/gen/choicemap_test.cljc

+  "Tests for the [[gen.choicemap]] namespace and the various base implementations
+  that live there."
+  (:require [clojure.test :refer [deftest is testing]]
+            [clojure.test.check.generators :as gen]


Does this mean gen throughout the tests refers to clojure.test.check.generators (not Gen the PPL)?

yup, correct, confusing here I guess but I think that's been their convention... that's not going to work in other namespaces though so I should change it!

georgematheos · 2023-11-16T01:40:05Z

test/gen/generators.cljc

+             (gen/map k-gen v-gen))))
+
+(defn gen-vector-choicemap
+  "Returns a generator that produces [[gen.choicemap/Choice]] instances."


docstring typo

fixed, thank you!

georgematheos · 2023-11-16T01:47:26Z

test/gen/choicemap_test.cljc

+  (checking "not-found arity" 100
+            [m (generators/gen-dynamic-choicemap)]
+            (is (= ::not-found
+                   (m ::not-present ::not-found))))


any pointers to where I can learn what ::not-found and ::not-present mean?

those are "namespaced keywords": https://blog.jeaye.com/2017/10/31/clojure-keywords/

So ::not-found expands out to :gen.choicemap-test/not-found. I use it here because the generative testing library is going to generate many random keywords and I want to make sure that my not-found "sentinel" doesn't clash with any of those.

georgematheos · 2023-11-16T01:50:45Z

test/gen/choicemap_test.cljc

+    (is (choicemap/empty? []))
+    (is (choicemap/empty? {})))
+
+  (testing "merge"


Might be good to add a test for merging hierarchical choicemaps?
E.g. {:k 1} with {:a {:b 1}}, and {:k 1} with {:k {:a 1}}. (Is the second case supposed to be an error?)

added, and yes!

sritchie

Great comments, hopefully these responses help!

sritchie · 2023-11-17T18:07:54Z

src/gen/choicemap.cljc

+  arr/IArray
+  (to-array [_] [v])
+  (-from-array [_ xs idx]
+    [1 (Choice. (nth xs idx))])


keep questions like this coming! also we should get a REPL up and pair soon.

This function wants a return value of a 2-element vector with

how many items you took off of the input array xs

the new value you created with those elements

As described here https://github.com/InferenceQL/gen.clj/blob/main/src/gen/array.cljc#L10

So for Choice, we take 1 thing off from xs at start-idx, and create the Choice instance in the second slot.

(And yes you're right that it's creating the vector [1, Choice(,,,,)]

sritchie · 2023-11-17T18:08:55Z

src/gen/choicemap.cljc

+  (-from-array [_ xs idx]
+    [1 (Choice. (nth xs idx))])
+
+  #?@(:clj


It is surprisingly annoying in Clojure to extend the base interfaces!! these are flexed in the tests, and I tried to match the implementations in Gen.jl for things like getindex. I'll comment below on what these do

sritchie · 2023-11-17T18:09:05Z

src/gen/choicemap.cljc

+
+  #?@(:clj
+      [Object
+       (toString [this] (pr-str this))


basic equality and stringification

sritchie · 2023-11-17T18:09:36Z

src/gen/choicemap.cljc

+       (equiv [this other] (-equiv this other))
+
+       IPrintWithWriter
+       (-pr-writer [_ writer opts]


ClojureScript is more modular with its interfaces; this is how ClojureScript prints, and the interface below is more generic equality than the js equality above.

sritchie · 2023-11-17T18:09:56Z

src/gen/choicemap.cljc

+               (and (instance? Choice o)
+                    (= v (.-v ^Choice o))))]))
+
+#?(:cljs


the next two methods use our print implementation with clojure's default printer and pretty printer.

sritchie · 2023-11-17T18:42:11Z

src/gen/choicemap.cljc

+  [v]
+  (if (choicemap? v)
+    (and (not (has-value? v))
+         (core/empty? v))


I think empty? actually calls seq and sees if it returns nil. The empty function WITHOUT the question mark returns an empty instance of the supplied type:

gen.choicemap-test> (empty {1 2 3 4}) {} gen.choicemap-test> (empty [1 2 3]) [] gen.choicemap-test> (empty '(1 2 3)) () gen.choicemap-test> (empty (choicemap/choicemap [1 2 3])) #gen/choicemap {}

sritchie · 2023-11-17T18:42:40Z

test/gen/choicemap_test.cljc

+  "Tests for the [[gen.choicemap]] namespace and the various base implementations
+  that live there."
+  (:require [clojure.test :refer [deftest is testing]]
+            [clojure.test.check.generators :as gen]


yup, correct, confusing here I guess but I think that's been their convention... that's not going to work in other namespaces though so I should change it!

sritchie · 2023-11-17T18:44:21Z

test/gen/choicemap_test.cljc

+  (checking "not-found arity" 100
+            [m (generators/gen-dynamic-choicemap)]
+            (is (= ::not-found
+                   (m ::not-present ::not-found))))


those are "namespaced keywords": https://blog.jeaye.com/2017/10/31/clojure-keywords/

So ::not-found expands out to :gen.choicemap-test/not-found. I use it here because the generative testing library is going to generate many random keywords and I want to make sure that my not-found "sentinel" doesn't clash with any of those.

sritchie · 2023-11-17T18:47:00Z

test/gen/choicemap_test.cljc

+    (is (choicemap/empty? []))
+    (is (choicemap/empty? {})))
+
+  (testing "merge"


added, and yes!

sritchie · 2023-11-17T18:48:04Z

test/gen/generators.cljc

+             (gen/map k-gen v-gen))))
+
+(defn gen-vector-choicemap
+  "Returns a generator that produces [[gen.choicemap/Choice]] instances."


fixed, thank you!

sritchie · 2023-11-17T18:51:21Z

@georgematheos good Q... I am going to delete gen/choice_map.cljc in favor of this, after a final PR that converts the dynamic language implementation pieces over to this new stuff + #54 . This is the future!

georgematheos · 2023-11-17T20:28:24Z

Thanks for the detailed responses -- looks great!

sritchie requested a review from zane November 14, 2023 19:57

sritchie changed the title ~~feat: new choicemap interface~~ feat: new IChoiceMap interface, implementations Nov 14, 2023

sritchie force-pushed the sritchie/choicemap branch from fa56db4 to 39c82af Compare November 14, 2023 20:22

sritchie force-pushed the sritchie/array branch from 70fbc5e to 40d408f Compare November 14, 2023 21:08

Base automatically changed from sritchie/array to main November 14, 2023 21:08

sritchie force-pushed the sritchie/choicemap branch from 39c82af to 11113f9 Compare November 14, 2023 21:09

sritchie changed the title ~~feat: new IChoiceMap interface, implementations~~ feat: new IChoiceMap interface, implementations (2/n) Nov 14, 2023

georgematheos reviewed Nov 16, 2023

View reviewed changes

feat: new choicemap interface

9a75b78

sritchie force-pushed the sritchie/choicemap branch from 11113f9 to 9a75b78 Compare November 17, 2023 18:48

sritchie commented Nov 17, 2023

View reviewed changes

zane approved these changes Nov 17, 2023

View reviewed changes

sritchie merged commit 868346a into main Nov 18, 2023
6 checks passed

sritchie deleted the sritchie/choicemap branch November 18, 2023 01:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: new IChoiceMap interface, implementations (2/n) #52

feat: new IChoiceMap interface, implementations (2/n) #52

sritchie commented Nov 14, 2023 •

edited

Loading

codecov bot commented Nov 14, 2023 •

edited

Loading

georgematheos left a comment

georgematheos Nov 16, 2023

georgematheos Nov 16, 2023

sritchie Nov 17, 2023

georgematheos Nov 16, 2023

sritchie Nov 17, 2023

georgematheos Nov 16, 2023

georgematheos Nov 16, 2023

georgematheos Nov 16, 2023

sritchie Nov 17, 2023

georgematheos Nov 16, 2023

sritchie Nov 17, 2023

georgematheos Nov 16, 2023

sritchie Nov 17, 2023

georgematheos Nov 16, 2023

sritchie Nov 17, 2023

georgematheos Nov 16, 2023

sritchie Nov 17, 2023

georgematheos Nov 16, 2023

sritchie Nov 17, 2023

sritchie left a comment

sritchie Nov 17, 2023

sritchie Nov 17, 2023

sritchie Nov 17, 2023

sritchie Nov 17, 2023

sritchie Nov 17, 2023

sritchie Nov 17, 2023

sritchie Nov 17, 2023

sritchie Nov 17, 2023

sritchie Nov 17, 2023

sritchie Nov 17, 2023

sritchie commented Nov 17, 2023

georgematheos commented Nov 17, 2023

feat: new IChoiceMap interface, implementations (2/n) #52

feat: new IChoiceMap interface, implementations (2/n) #52

Conversation

sritchie commented Nov 14, 2023 • edited Loading

codecov bot commented Nov 14, 2023 • edited Loading

Codecov Report

georgematheos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sritchie left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sritchie commented Nov 17, 2023

georgematheos commented Nov 17, 2023

sritchie commented Nov 14, 2023 •

edited

Loading

codecov bot commented Nov 14, 2023 •

edited

Loading