Added copy constructor, move constructor and copy assignment operator. #826

s1998 · 2016-12-11T22:04:03Z

I tried to add copy constructor, move constructor and copy assignment operator to DTree.
Function declarations are present in dtree.hpp and function definitions are added in dtree_impl.hpp
Token number #817 .
Waiting for review and suggestions to fix the issue.

rcurtin · 2016-12-12T16:46:39Z

Hi there,

Thanks for the contribution. Could you please add some tests for these? Take a look at some tests I recently wrote in a different branch: https://github.com/rcurtin/mlpack/blob/bindings/src/mlpack/tests/lsh_test.cpp#L840

Once there are working tests I'll give it a full review.

…perator and move assignment operator.

s1998 · 2016-12-15T17:14:25Z

@rcurtin
I added the tests.

rcurtin

Thanks for adding the tests. There are still a couple issues to take care of before we can merge this, though.

rcurtin · 2016-12-20T21:03:35Z

src/mlpack/methods/det/dtree_impl.hpp

+
+    //Copying the children 
+    left = obj.left;
+    right = obj.right;


Couldn't you simplify this to left = new DTree(*obj.left); and the same for the right?

rcurtin · 2016-12-20T21:07:08Z

src/mlpack/tests/det_test.cpp

+  BOOST_REQUIRE(tree2.Left()==leftChild);
+  BOOST_REQUIRE(tree2.Right()==rightChild);
+  BOOST_REQUIRE(tree3.Left()==leftChild);
+  BOOST_REQUIRE(tree3.Right()==rightChild);


At this point leftChild and rightChild should be invalid pointers; I bet valgrind will call this an invalid memory access and in some cases this test will fail.

rcurtin · 2016-12-20T21:07:20Z

src/mlpack/tests/det_test.cpp

+
+  //Checking children were safely moved
+  BOOST_REQUIRE(tree2.Left()==leftChild);
+  BOOST_REQUIRE(tree2.Right()==rightChild);


Same issue with leftChild and rightChild here.

rcurtin · 2016-12-20T21:09:37Z

src/mlpack/methods/det/dtree_impl.hpp

+template <typename MatType, typename TagType>
+DTree<MatType, TagType>& DTree<MatType, TagType>::operator=(const DTree<MatType, TagType>& obj)
+{
+    if (this != &obj){


I don't think these checks are necessary. If that is true, the user is doing something pretty stupid and we shouldn't need to handle it.

…d move assignment operator.

s1998 · 2016-12-22T13:53:09Z

@rcurtin
I tried to fix the issues. The checks are still failing. Travis-ci says that RAModel test failed (I was unable to find how dtree and knn is related. If you could tell me about that, I would try to fix the issue).

rcurtin · 2016-12-27T16:20:17Z

src/mlpack/methods/det/dtree_impl.hpp

+    alphaUpper(obj.alphaUpper),
+    left(obj.left),
+    right(obj.right)
+{ /* Nothing to do. */ }


This doesn't actually copy the left and right child nodes, but your copy operator does. Can you fix that please?

@rcurtin The child node (data) needs to be copied or the pointer to the child node?

This is the copy constructor, so you need to copy the data. Take a look at the copy constructor test you wrote---if you just copy the pointer value, then when you delete the tree you copied from, suddenly the left and right pointers point to invalid memory. I bet that if you run valgrind to check for memory leaks on that test, like

$ bin/mlpack_test -t DETTest/CopyConstructorAndOperatorTest

then valgrind will throw invalid memory access errors everywhere, even if the test itself does not fail.

rcurtin · 2016-12-27T16:20:52Z

src/mlpack/methods/det/dtree_impl.hpp

+    obj.bucketTag = -1;
+    obj.alphaUpper = 0.0;
+    obj.left = NULL;
+    obj.right = NULL;


You should be able to do this as an initialization list.

I actually had seen those links before creating the PR and had used initialization list in copy constructor. I saw them again now. What confused me was that at line 249 you commented that one can use initialization list for the same . But there we are trying to set the default values of the field of object passed (obj - the one on RHS ) and not of the object created.
What I understood from the link was that initialization list can be used to initialize the fields of the object that is being created (the one for which the constructor has been called - on LHS).
I think I haven't still understood what exactly was said here. I have tried to fix other bugs.

Yeah---you can use an initialization list in this method to set the values of the object being constructed. You are right that you can't do that with the RHS, but that isn't what I meant. Sorry for the confusion.

rcurtin · 2016-12-27T16:21:18Z

src/mlpack/methods/det/dtree_impl.hpp

+template <typename MatType, typename TagType>
+DTree<MatType, TagType>& DTree<MatType, TagType>::operator=(DTree<MatType, TagType>&& obj)
+{
+    if (this != &obj){  


I think the test to see if this is the same as the object being copied is unnecessary.

rcurtin · 2016-12-27T17:14:22Z

src/mlpack/methods/det/dtree_impl.hpp

+    ratio = std::move(obj.ratio);
+    logVolume = std::move(obj.logVolume);
+    bucketTag = std::move(obj.bucketTag);
+    alphaUpper = std::move(obj.alphaUpper);


I wouldn't bother with std::move() for primitive types like int, double, etc., since move will degrade to copies for primitive types. As a result, using move() there can actually make the code more confusing, because we can't assume that the rvalues will be set to default values or something. (That is my current understanding of the spec.)

… copied properly

s1998 · 2017-01-10T13:39:53Z

@rcurtin
I tried to fix the issues and changed the tests to ensure children along with their data are properly copied.
I don't know why the appveyor still fails.
bin/mlpack_test -t DETTest/CopyConstructorAndOperatorTest
bin/mlpack_test -t DETTest/MoveOperatorTest
bin/mlpack_test -t DETTest/MoveConstructorTestgive positive results on my system.

rcurtin · 2017-01-11T16:50:47Z

src/mlpack/methods/det/dtree_impl.hpp

+    if(obj.right != NULL)
+      right = new DTree(*obj.right);
+    else
+      right = NULL;


You can write these in the initialization list cleverly: left((obj.left == NULL) ? NULL : new DTree(*obj.left)), for instance.

rcurtin · 2017-01-11T16:51:06Z

src/mlpack/methods/det/dtree_impl.hpp

+    if(obj.right != NULL)
+      right = new DTree(*obj.right);
+    else
+      right = NULL;


What happens when left and right are not NULL when this method is called?

I think they are supposed to be freed.

rcurtin · 2017-01-11T16:52:26Z

src/mlpack/methods/det/dtree_impl.hpp

+
+    //Moving children
+    left = std::move(obj.left);
+    right = std::move(obj.right);


Since you're just moving pointers here, you don't actually need to use std::move(), you can just do left = obj.left and right = obj.right.

rcurtin · 2017-01-11T16:55:04Z

The AppVeyor failure is a compilation error; if you dig through the log and search for "error " you can see what the issue is:

C:\projects\mlpack\src\mlpack\tests\det_test.cpp(516): error C2248: 'mlpack::det::DTree<arma::mat,int>::maxVals': cannot access private member declared in class 'mlpack::det::DTree<arma::mat,int>' [C:\projects\mlpack\build\src\mlpack\tests\mlpack_test.vcxp

So although gcc lets you get away with accessing a private member of DTree, MSVC doesn't. It seems like you'll have to find a different way to test that.

s1998 · 2017-01-12T07:39:07Z

@rcurtin I tried to fix the issues. (I should have gone through AppVeyor log more carefully.)

rcurtin · 2017-01-13T22:02:29Z

Thanks for taking care of all the issues. Did you have anything else to modify or should I merge this?

s1998 · 2017-01-13T22:07:35Z

I can't think of anything else to modify. If there are no issues left, may be this should be merged. :)

rcurtin · 2017-01-17T14:54:11Z

Ok, merged in. I made some style fixes in 547e2cb. Thanks for your hard work on this PR. Would you like to add your name and email to the list of contributors in src/mlpack/core.hpp and COPYRIGHT.txt?

s1998 · 2017-01-18T01:55:17Z

@rcurtin I would like to add my name and email to the list, would I need a different PR for that ?

rcurtin · 2017-01-18T02:00:49Z

Yeah---this one is already merged so you'll need to open another one for that.

s1998 · 2017-01-18T02:01:21Z

Okay .

Added copy constructor, move constructor and copy assignment operator.

8dd4845

s1998 added 2 commits December 15, 2016 00:10

Added tests for copy constructor, move constructor, copy assignment o…

5d5861e

…perator and move assignment operator.

Added tests for copy constructor, move constructor, copy assignment o…

952211b

…perator and move assignment operator.

rcurtin reviewed Dec 20, 2016

View reviewed changes

s1998 added 5 commits December 21, 2016 22:21

Added copy constructor, move constructor, copy assignment operator an…

a1a0e3c

…d move assignment operator.

Added copy constrcutor, move constructor, copy assignment operator an…

a1a1cef

…d move assignment operator.

Update dtree_impl.hpp

4fccbc9

Update dtree_impl.hpp

89cef13

Update det_test.cpp

7d1bb44

rcurtin reviewed Dec 27, 2016

View reviewed changes

s1998 added 3 commits January 6, 2017 18:42

Update dtree_impl.hpp

3a3fd60

Updated copy constructor and changed the tests to ensure children are…

ddbfd81

… copied properly

Updated copy constructor and changed the tests to ensure children are…

a7ff69c

… copied properly

rcurtin reviewed Jan 11, 2017

View reviewed changes

s1998 added 5 commits January 12, 2017 07:37

Update copy/move constuctor amd operator.

5542ae9

Update dtree_impl.hpp

2e37a38

Update det_test.cpp

58cf0d9

Merge branch 'master' of https://github.com/s1998/mlpack

c9e7fb0

Update copy/move constuctor amd operator.

f709ad6

rcurtin merged commit 6c952f7 into mlpack:master Jan 17, 2017

This was referenced Jan 17, 2017

Copy, move constructors and copy assignment operator added #824

Closed

Implement DTree copy constructor, move constructor, and copy/move assignment operators #817

Closed

rcurtin modified the milestones: mlpack 2.2.0, mlpack 3.0.0 Mar 17, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added copy constructor, move constructor and copy assignment operator. #826

Added copy constructor, move constructor and copy assignment operator. #826

s1998 commented Dec 11, 2016 •

edited

rcurtin commented Dec 12, 2016

s1998 commented Dec 15, 2016 •

edited

rcurtin left a comment

rcurtin Dec 20, 2016

rcurtin Dec 20, 2016

rcurtin Dec 20, 2016

rcurtin Dec 20, 2016

s1998 commented Dec 22, 2016

rcurtin Dec 27, 2016

s1998 Jan 9, 2017 •

edited

rcurtin Jan 9, 2017

rcurtin Dec 27, 2016

s1998 Jan 5, 2017 •

edited

rcurtin Jan 9, 2017

rcurtin Dec 27, 2016

rcurtin Dec 27, 2016

s1998 commented Jan 10, 2017 •

edited

rcurtin Jan 11, 2017

rcurtin Jan 11, 2017

s1998 Jan 12, 2017

rcurtin Jan 11, 2017

rcurtin commented Jan 11, 2017

s1998 commented Jan 12, 2017 •

edited

rcurtin commented Jan 13, 2017

s1998 commented Jan 13, 2017 •

edited

rcurtin commented Jan 17, 2017

s1998 commented Jan 18, 2017

rcurtin commented Jan 18, 2017

s1998 commented Jan 18, 2017

Added copy constructor, move constructor and copy assignment operator. #826

Added copy constructor, move constructor and copy assignment operator. #826

Conversation

s1998 commented Dec 11, 2016 • edited

rcurtin commented Dec 12, 2016

s1998 commented Dec 15, 2016 • edited

rcurtin 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

s1998 commented Dec 22, 2016

Choose a reason for hiding this comment

s1998 Jan 9, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

s1998 Jan 5, 2017 • edited

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

s1998 commented Jan 10, 2017 • edited

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

rcurtin commented Jan 11, 2017

s1998 commented Jan 12, 2017 • edited

rcurtin commented Jan 13, 2017

s1998 commented Jan 13, 2017 • edited

rcurtin commented Jan 17, 2017

s1998 commented Jan 18, 2017

rcurtin commented Jan 18, 2017

s1998 commented Jan 18, 2017

s1998 commented Dec 11, 2016 •

edited

s1998 commented Dec 15, 2016 •

edited

s1998 Jan 9, 2017 •

edited

s1998 Jan 5, 2017 •

edited

s1998 commented Jan 10, 2017 •

edited

s1998 commented Jan 12, 2017 •

edited

s1998 commented Jan 13, 2017 •

edited