Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Modify dissoc-in to accept variable args #25

Merged
merged 1 commit into from Jan 17, 2018

Conversation

rackdon
Copy link

@rackdon rackdon commented Dec 17, 2017

This pr solves issue #23
Through this change we can use the dissoc-in function like in the following example

(medley.core/dissoc-in {:a {:b 3 :c 4 :d 5}} [:a :b] [:a :c])

@weavejester
Copy link
Owner

You need to keep the original [m ks] arity around for performance purposes. Also, can you run a few criterium benchmarks before and after?

@weavejester
Copy link
Owner

Also, is reduce going to be faster, or is an explicit recur?

@rackdon
Copy link
Author

rackdon commented Dec 17, 2017

Why do you say it? You need to keep the original [m ks] arity around for performance purposes. I mean, I save that inside each iteration.
On the other hand I'll obtain the different benchmarks using reduce and recur because I think reduce is faster but I have to ensure that

@weavejester
Copy link
Owner

In your changes, you've removed the [m ks] arity, which means that if someone runs (dissoc-in m ks) it will fall to the [m & ks] arity instead. This means that Clojure needs to wrap ks in a seq, and then perform a reduce on it. We can avoid both of these things by having an explicit [m ks] arity.

@rackdon
Copy link
Author

rackdon commented Dec 25, 2017

With the following element {:a {:b {:c 1} :d 2} :b {:c {:d 2 :e 3}}} using dissoc-in with [:a :b :c] obtaining the average value in miliseconds over 1000000 iterations and executing five times each trial.

old dissoc in -> 0.005182, 0.005265, 0.005219, 0.005197, 0.005223
new dissoc in with reduce -> 0.006803, 0.006727, 0.006853, 0.007013, 0.006691
new dissoc in with loop recur -> 0.005548, 0.005634, 0.005491, 0.005542, 0.005499

With the following element {:a {:b {:c 1} :d 2} :b {:c {:d 2 :e 3}}} using dissoc-in with [:a :b :c] [:b :c :d] obtaining the average value in miliseconds over 1000000 iterations and executing five times each trial.

new dissoc in with reduce -> 0.012029, 0.011852, 0.011736, 0.011868, 0.011746
new dissoc in with loop recur -> 0.011544, 0.011476, 0.011493, 0.011362, 0.01154

@weavejester
Copy link
Owner

Thanks for the tests. It looks like it's more performant to use a loop-recur, though if you could use Criterium's criterium.core/quick-bench function instead, that will give you more accurate results.

(dissoc m k))
m))
([m ks & rest-ks]
(loop [m m
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm pretty sure you can get rid of this loop and just recur over the function body. Take a look at dissoc for reference.

@weavejester
Copy link
Owner

weavejester commented Jan 6, 2018

I think you've misunderstood me. I was suggesting something like:

(defn dissoc-in
  ([m ks] 
   (if-let [[k & ks] (seq ks)]
     (if (seq ks)
       (let [v (dissoc-in (get m k) ks)]
         (if (empty? v)
           (dissoc m k)
           (assoc m k v)))
       (dissoc m k))
     m))
  ([m ks & kss] 
   (if-let [[ks' & kss] (seq kss)]
     (recur (dissoc-in m ks) ks' kss)
     (dissoc-in m ks))))

@weavejester
Copy link
Owner

Thanks! Could you squash your commits? Perhaps into something like:

Add variable arity to dissoc-in function

@rackdon
Copy link
Author

rackdon commented Jan 10, 2018

done

@weavejester
Copy link
Owner

Thanks! Can you update the commit message, too?

@rackdon
Copy link
Author

rackdon commented Jan 13, 2018

done

@weavejester weavejester merged commit 65a6925 into weavejester:master Jan 17, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants