Feature/map functions #350

shreyans800755 · 2017-05-25T19:41:32Z

Added intersection and difference functions for map along with docs.
Closes #341

shreyans800755 · 2017-05-25T20:41:29Z

@ldionne I think I should also add symmetric_difference implementation for maps. I'll try to update the PR with its commits soon.
EDIT: I think same implementation of sym_diff should work for both maps and sets.

One doubt:
Current implementation:
symmetric_difference(x, y) = union(difference(x, y), difference(y, x))

As you must be aware of the other implementation:
symmetric_difference(x, y) = difference(union(x, y), intersection(x, y))

Is former implementation is chosen randomly, or is it faster than later one ?

ldionne

Overall great, with a few nitpicks! Thanks a lot for the great PR!

As for symmetric_difference, I think it would be quite nice to add it in this PR. I'm not sure why I chose one implementation over the other when I wrote it for hana::set.

ldionne · 2017-06-06T16:54:05Z

example/map/difference.cpp

+// Distributed under the Boost Software License, Version 1.0.
+// (See accompanying file LICENSE.md or copy at http://boost.org/LICENSE_1_0.txt)
+
+#include <boost/hana/assert.hpp>


Please order includes.

ldionne · 2017-06-06T16:57:29Z

example/map/intersection.cpp

+// Distributed under the Boost Software License, Version 1.0.
+// (See accompanying file LICENSE.md or copy at http://boost.org/LICENSE_1_0.txt)
+
+#include <boost/hana/assert.hpp>


ldionne · 2017-06-06T16:59:08Z

include/boost/hana/fwd/map.hpp

+    //! is present in `ys`.
+    //! In other words, the following holds for any object `pair(k, v)`:
+    //! @code
+    //!     pair(k, v) ^in^ intersection(xs, ys) if and only if (k, v) ^in^ xs && k ^in^ ys.keys()


ys.keys() does not exist; consider using keys(ys) instead

ldionne · 2017-06-06T17:03:56Z

include/boost/hana/fwd/map.hpp

+    //!
+    //!
+    //! @note
+    //! This function is not commutative, i.e. `intersection(xs, ys)` is


Please reword to

//! This function is not commutative, i.e. `intersection(xs, ys)` is not //! necessarily the same as `intersection(ys, xs)`. Indeed, the set of keys //! in `intersection(xs, ys)` is always the same as the set of keys in //! `intersection(ys, xs)`, but the value associated to each key may be //! different. `intersection(xs, ys)` contains values present in `xs`, and //! `intersection(ys, xs)` contains values present in `ys`.

ldionne · 2017-06-06T17:04:31Z

include/boost/hana/fwd/map.hpp

+    //! is not present in `keys(ys)`.
+    //! In other words, the following holds for any object `pair(k, v)`:
+    //! @code
+    //!     pair(k, v) ^in^ difference(xs, ys) if and only if (k, v) ^in^ xs && k ^not in^ ys.keys()


Same comment for ys.keys().

ldionne · 2017-06-06T17:11:29Z

include/boost/hana/map.hpp

+
+            // Second template param will be pair
+            // Get its key and check if it exists, if it does, insert key, value pair.
+            template<typename Result, typename Pair>


Space between template and <typename.

ldionne · 2017-06-06T17:11:35Z

include/boost/hana/map.hpp

+                return hana::insert(static_cast<Result&&>(result), static_cast<Pair&&>(pair));
+            }
+
+            template<typename Result, typename Pair>


ldionne · 2017-06-06T17:12:03Z

test/map/difference.cpp

+#include <laws/base.hpp>
+#include <support/minimal_product.hpp>
+namespace hana = boost::hana;
+using hana::test::ct_eq;


I don't think you need this.

ldionne · 2017-06-06T17:13:58Z

test/map/difference.cpp

+                           p<1, 4>())
+        ),
+        hana::make_map()
+    ));


Can you add a bigger test case with something like 10 elements in the maps?

ldionne · 2017-06-06T17:14:10Z

test/map/intersection.cpp

+#include <laws/base.hpp>
+#include <support/minimal_product.hpp>
+namespace hana = boost::hana;
+using hana::test::ct_eq;


Not needed.

ricejasonf · 2017-06-06T19:12:53Z

What is the reason for intersection not being commutative. Is it a performance thing?

shreyans800755 · 2017-06-06T20:35:00Z

@ricejasonf intersection is done on the base of keys in case of hana::map, and not based on key and values, both. Frankly, I only thought this use case, and it didn't really struck into my head about intersecting two maps based on (key, value) pairs.

Is it a performance thing?

Performance based on asymptotic analysis should be same if want to intersect based on (key, value), although the overhead of comparison of value will be increased.

ricejasonf · 2017-06-06T20:50:58Z

@shreyans800755 Ah, right, it compares the keys. I wasn't even thinking about that. lol 😅

ldionne · 2017-06-08T19:57:48Z

One of the jobs failed with

/home/travis/build/boostorg/hana/include/boost/hana/fwd/set.hpp:263: warning: included file example/set/difference.cpp is not found. Check your EXAMPLE_PATH

ldionne · 2017-06-09T04:03:10Z

include/boost/hana/map.hpp

@@ -51,9 +53,11 @@ Distributed under the Boost Software License, Version 1.0.
 #include <boost/hana/optional.hpp>
 #include <boost/hana/remove_if.hpp>
 #include <boost/hana/second.hpp>
+#include <boost/hana/tuple.hpp>


Do you need #include <boost/hana/tuple.hpp> here?

Corrected.
I'll push symmetric_difference soon.

ldionne · 2017-06-11T18:29:08Z

example/symmetric_difference.cpp

@@ -5,10 +5,14 @@
 #include <boost/hana/assert.hpp>


Just like we did for the other functions, would it be possible to have an example/set/symmetric_difference.cpp and an example/map/symmetric_difference.cpp file? This would be consistent with the rest of the lib, too.

I thought of that, too. I chose this way because we don't have different implementations for set and map.

ldionne · 2017-06-11T18:29:37Z

test/symmetric_difference.cpp

@@ -0,0 +1,134 @@
+// Copyright Louis Dionne 2013-2017


Same comment as for examples; it would be nice to have a test for map and a test for sets in different files.

Same thought as example. So, I wasn't sure if we need two different files.

shreyans800755 · 2017-06-11T18:44:21Z

Regarding documentation of symmetric_difference, currently it exists only for set. Should I move it to include/boost/hana/fwd/set.hpp and add similar for map in include/boost/hana/fwd/map.hpp ?
Or may be just adding a @relatealso tag with map in the same file include/boost/hana/fwd/symmetric_difference.hpp should do ?

ldionne · 2017-06-11T19:53:34Z

I would duplicate the documentation for set and map. While duplication is generally bad, the rationale here is that the two implementations for set and map are conceptually unrelated, since they don't share a common concept. If we were to create a new concept and move symmetric_difference under it (and also probably others), then this is how we would de-duplicate the documentation. Does that make sense?

shreyans800755 · 2017-06-11T20:54:39Z

That makes sense. I'll push the changes.

ldionne · 2017-06-16T16:05:20Z

test/set/symmetric_difference.cpp

@@ -2,8 +2,6 @@
 // Distributed under the Boost Software License, Version 1.0.
 // (See accompanying file LICENSE.md or copy at http://boost.org/LICENSE_1_0.txt)

-#include <boost/hana/assert.hpp>


Those should stay, no?

It is already coming from:
test/set/symmetric_difference.cpp:10
test/_include/laws/base.hpp:25
test/_include/support/tracked.hpp:11

And that's why tests are also passing.

Yeah, but you should always include what you use to avoid relying on such dependencies, no?

Makes sense. Will update soon.

Closes boostorg#341

ricejasonf · 2017-06-24T20:07:27Z

👍

ldionne · 2017-06-24T20:08:30Z

Thanks a lot @shreyans800755, this is a great PR!

shreyans800755 force-pushed the feature/map_functions branch from 68f5bc4 to 6f9d96e Compare May 25, 2017 19:59

shreyans800755 force-pushed the feature/map_functions branch from 6f9d96e to 42944a8 Compare May 27, 2017 12:52

ldionne requested changes Jun 6, 2017

View reviewed changes

shreyans800755 force-pushed the feature/map_functions branch 2 times, most recently from f8bc26f to a697773 Compare June 8, 2017 16:59

shreyans800755 force-pushed the feature/map_functions branch from a697773 to c7e2b48 Compare June 8, 2017 20:19

ldionne reviewed Jun 9, 2017

View reviewed changes

shreyans800755 force-pushed the feature/map_functions branch from c7e2b48 to 71858c6 Compare June 9, 2017 17:12

ldionne reviewed Jun 11, 2017

View reviewed changes

shreyans800755 force-pushed the feature/map_functions branch from 5a7a216 to 311b052 Compare June 11, 2017 20:51

shreyans800755 force-pushed the feature/map_functions branch from 311b052 to 3200dce Compare June 11, 2017 21:41

ldionne reviewed Jun 16, 2017

View reviewed changes

shreyans800755 added 6 commits June 19, 2017 01:08

[map] Add intersection method for map

074a5fa

Closes boostorg#341

[intersection] Document separately for map and set

dbf83ac

Closes boostorg#341

[map] Add difference method for map

d70b015

Closes boostorg#341

[difference] Document separately for map and set

1e6011d

Closes boostorg#341

[symmetric_difference] Add tests and example for map

220017b

Closes boostorg#341

[symmetric_difference] Document separately for map and set

a88133a

Closes boostorg#341

shreyans800755 force-pushed the feature/map_functions branch from 3200dce to a88133a Compare June 18, 2017 19:39

ldionne approved these changes Jun 24, 2017

View reviewed changes

ldionne merged commit 7533611 into boostorg:develop Jun 24, 2017

ldionne mentioned this pull request Jun 24, 2017

Add set-theoretic functions for hana::map #341

Closed

shreyans800755 deleted the feature/map_functions branch June 24, 2017 20:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/map functions #350

Feature/map functions #350

shreyans800755 commented May 25, 2017 •

edited

Loading

shreyans800755 commented May 25, 2017 •

edited

Loading

ldionne left a comment •

edited

Loading

ldionne Jun 6, 2017

ldionne Jun 6, 2017

ldionne Jun 6, 2017

ldionne Jun 6, 2017

ldionne Jun 6, 2017

ldionne Jun 6, 2017

ldionne Jun 6, 2017

ldionne Jun 6, 2017

ldionne Jun 6, 2017

ldionne Jun 6, 2017

ricejasonf commented Jun 6, 2017

shreyans800755 commented Jun 6, 2017

ricejasonf commented Jun 6, 2017 •

edited

Loading

ldionne commented Jun 8, 2017

ldionne Jun 9, 2017

shreyans800755 Jun 9, 2017

ldionne Jun 11, 2017

shreyans800755 Jun 11, 2017

ldionne Jun 11, 2017

shreyans800755 Jun 11, 2017 •

edited

Loading

shreyans800755 commented Jun 11, 2017

ldionne commented Jun 11, 2017

shreyans800755 commented Jun 11, 2017

ldionne Jun 16, 2017

shreyans800755 Jun 16, 2017 •

edited

Loading

ldionne Jun 18, 2017

shreyans800755 Jun 18, 2017

ricejasonf commented Jun 24, 2017

ldionne commented Jun 24, 2017

Feature/map functions #350

Feature/map functions #350

Conversation

shreyans800755 commented May 25, 2017 • edited Loading

shreyans800755 commented May 25, 2017 • edited Loading

ldionne left a comment • edited Loading

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

ricejasonf commented Jun 6, 2017

shreyans800755 commented Jun 6, 2017

ricejasonf commented Jun 6, 2017 • edited Loading

ldionne commented Jun 8, 2017

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

shreyans800755 Jun 11, 2017 • edited Loading

Choose a reason for hiding this comment

shreyans800755 commented Jun 11, 2017

ldionne commented Jun 11, 2017

shreyans800755 commented Jun 11, 2017

Choose a reason for hiding this comment

shreyans800755 Jun 16, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ricejasonf commented Jun 24, 2017

ldionne commented Jun 24, 2017

shreyans800755 commented May 25, 2017 •

edited

Loading

shreyans800755 commented May 25, 2017 •

edited

Loading

ldionne left a comment •

edited

Loading

ricejasonf commented Jun 6, 2017 •

edited

Loading

shreyans800755 Jun 11, 2017 •

edited

Loading

shreyans800755 Jun 16, 2017 •

edited

Loading