Static error: fix static error for list of classes

[rxng8]

Refer to issue #2798.
list of the errors and warnings (List taken from #2798 ) -

• mlpack/src/mlpack/methods/adaboost/adaboost_model.hpp 26 warn V690
  The 'AdaBoostModel' class implements a move constructor but lacks the move assignment operator. It is dangerous to use such a class.

• mlpack/src/mlpack/methods/adaboost/adaboost_model.cpp 74 warn V794
  The assignment operator should be protected from the case of 'this == &other'.

• mlpack/src/mlpack/core/tree/hrectbound.hpp 54 warn V690
  The 'HRectBound' class implements a move constructor, but lacks the move assignment operator. It is dangerous to use such a class.

• mlpack/src/mlpack/core/tree/ballbound.hpp 32 warn V690
  The 'BallBound' class implements a move constructor, but lacks the move assignment operator. It is dangerous to use such a class.

• mlpack/src/mlpack/core/tree/hollow_ball_bound.hpp 33 warn V690
  The 'HollowBallBound' class implements a move constructor, but lacks the move assignment operator. It is dangerous to use such a class.

• mlpack/src/mlpack/core/tree/hollow_ball_bound_impl.hpp 82 warn V794
  The assignment operator should be protected from the case of 'this == &other'.

• mlpack/src/mlpack/core/tree/cellbound.hpp 75 warn V690
  The 'CellBound' class implements a move constructor, but lacks the move assignment operator. It is dangerous to use such a class.

• mlpack/src/mlpack/core/tree/rectangle_tree/r_tree_split_impl.hpp 441 err V758
  The 'range' reference becomes invalid when temporary object returned by a function is destroyed.

• mlpack/src/mlpack/core/tree/rectangle_tree/discrete_hilbert_value.hpp 29 warn V690
  The 'DiscreteHilbertValue' class implements a move constructor, but lacks the move assignment operator. It is dangerous to use such a class.

• mlpack/src/mlpack/methods/amf/update_rules/svd_incomplete_incremental_learning.hpp 53 err V730
  Not all members of a class are initialized inside the constructor. Consider inspecting: currentUserIndex.

• mlpack/src/mlpack/methods/amf/update_rules/svd_complete_incremental_learning.hpp 56 err V730
  Not all members of a class are initialized inside the constructor. Consider inspecting: currentUserIndex, currentItemIndex.

• mlpack/src/mlpack/methods/amf/update_rules/svd_complete_incremental_learning.hpp 172 err V730
  Not all members of a class are initialized inside the constructor. Consider inspecting: n, m, isStart.

• mlpack/src/mlpack/methods/amf/termination_policies/simple_residue_termination.hpp 42 warn V730
  Not all members of a class are initialized inside the constructor. Consider inspecting: residue, iteration, normOld, nm.

• mlpack/src/mlpack/methods/range_search/range_search.hpp 42 warn V690
  The 'RangeSearch' class implements a move constructor, but lacks the move assignment operator. It is dangerous to use such a class.

• mlpack/src/mlpack/methods/range_search/range_search_impl.hpp 261 warn V1004
  The 'referenceTree' pointer was used unsafely after it was verified against nullptr. Check lines: 257, 261.

• mlpack/src/mlpack/methods/fastmks/fastmks.hpp 63 warn V690
  The 'FastMKS' class implements a move constructor, but lacks the move assignment operator. It is dangerous to use such a class.

• mlpack/src/mlpack/methods/fastmks/fastmks_model.hpp 34 warn V690
  The 'FastMKSModel' class implements a move constructor, but lacks the move assignment operator. It is dangerous to use such a class.

• mlpack/src/mlpack/methods/fastmks/fastmks_model.cpp 73 warn V794
  The assignment operator should be protected from the case of 'this == &other'.

• mlpack/src/mlpack/methods/hmm/hmm_model.hpp 33 warn V690
  The 'HMMModel' class implements a move constructor, but lacks the move assignment operator. It is dangerous to use such a class.

• mlpack/src/mlpack/methods/hoeffding_trees/hoeffding_tree.hpp 61 warn V690
  The 'HoeffdingTree' class implements a copy constructor, but lacks the copy assignment operator. It is dangerous to use such a class.

• mlpack/src/mlpack/methods/hoeffding_trees/hoeffding_tree_model.cpp 64 warn V794
  The assignment operator should be protected from the case of 'this == &other'.

• mlpack/src/mlpack/methods/hoeffding_trees/hoeffding_tree_model.cpp 92 warn V794
  The assignment operator should be protected from the case of 'this == &other'.

• mlpack/src/mlpack/methods/kde/kde.hpp 88 warn V690
  The 'KDE' class implements a move constructor, but lacks the move assignment operator. It is dangerous to use such a class.

• mlpack/src/mlpack/methods/kmeans/dual_tree_kmeans_rules_impl.hpp 160 warn V519
  The 'adjustedScore' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 159, 160.

• mlpack/src/mlpack/methods/kmeans/dual_tree_kmeans_rules_impl.hpp 21 err V730
  Not all members of a class are initialized inside the constructor. Consider inspecting: lastBaseCase.

• mlpack/src/mlpack/methods/lars/lars.cpp 420 warn V688
  The 'activeSet' local variable possesses the same name as one of the class members, which can result in a confusion.

• mlpack/src/mlpack/methods/preprocess/scaling_model.hpp 29 warn V690
  The 'ScalingModel' class implements a move constructor, but lacks the move assignment operator. It is dangerous to use such a class.

• mlpack/src/mlpack/methods/ann/layer/reparametrization_impl.hpp 68 err V562
  It's odd to compare a bool type value with a value of double type.

• mlpack/src/mlpack/methods/ann/layer/reinforce_normal_impl.hpp 23 warn V730
  Not all members of a class are initialized inside the constructor. Consider inspecting: reward, deterministic.

• mlpack/src/mlpack/methods/ann/layer/recurrent_attention_impl.hpp 30 err V730
  Not all members of a class are initialized inside the constructor. Consider inspecting: outSize.

• mlpack/src/mlpack/methods/reinforcement_learning/q_networks/categorical_dqn.hpp 56 warn V730
  Not all members of a class are initialized inside the constructor. Consider inspecting: atomSize, vMin, vMax.

• mlpack/src/mlpack/methods/amf/termination_policies/incomplete_incremental_termination.hpp 36 warn V730
  Not all members of a class are initialized inside the constructor. Consider inspecting: incrementalIndex, iteration.

• mlpack/src/mlpack/methods/amf/termination_policies/complete_incremental_termination.hpp 37 warn V730
  Not all members of a class are initialized inside the constructor. Consider inspecting: incrementalIndex, iteration.

[rxng8]

@rcurtin Hi, as I am checking the static error, I encounter an error about calculating the adjustedScore in dual_tree_kmeans_rules_impl.hpp, the error said "The 'adjustedScore' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 160, 161." Check here

Basically, the adjusted score is computed 2 times:

adjustedScore = lastScore + lastQueryDescDist;
adjustedScore = lastScore + lastRefDescDist;

In the comment you said that "the last score is equal to the distance between the centroids minus the radii of the query and reference bounds along the axis of the line between the two centroids." So can I fix it like this?

adjustedScore = lastScore + lastQueryDescDist + lastRefDescDist;

[rcurtin]

@rxng8 I think your suggestion is correct. It looks like the current implementation is definitely a bug. If you change to the code you proposed, do all the tests still pass?

[rxng8]

@rcurtin Yes, all the [KMeansTest] still passed!

[rcurtin]

@rxng8 awesome! Then I think that is the correct fix. 👍

[rxng8]

@zoq Hi, I have finished the fix. Please feel free to review it!

[zoq]

Looks like there are two issues that aren't checked yet, but I guess you checked everything?

[rxng8]

@zoq As you can see from this static code check (from @gauravghati, issue #2798 ) which includes the reparametrization static error and the lars static error.

• For the reparametrization static error, there is no comparison in the error line (and also no parenthesis), so we cannot conclude what happened there. (Maybe it is a bug of the static code checker?)
• for the lars static error, the class variable activeSet is intended to be there to be modified, there are no other method parameters called activeSet as well as predefined variable activeSet or assignment of the activeSet variable. So everything is fine! (Maybe it is a bug of the static code checker?)

Therefore, I believe that it is fine now, but I will still leave those unchecked!

[zoq]

@zoq As you can see from this static code check (from @gauravghati, issue #2798 ) which includes the reparametrization static error and the lars static error.

* For the `reparametrization` static error, there is no comparison in the error line (and also no parenthesis), so we cannot conclude what happened there. (Maybe it is a bug of the static code checker?)

* for the `lars` static error, the class variable `activeSet` is intended to be there to be modified, there are no other method parameters called `activeSet` as well as predefined variable `activeSet` or assignment of the `activeSet` variable. So everything is fine! (Maybe it is a bug of the static code checker?)

Therefore, I believe that it is fine now, but I will still leave those unchecked!

Okay, perfect, I looked at the two issues and I think they are false postives, so safe to ignore at this point.

[zoq]

Just some really minor style issues; the rest looks good.

[rxng8]

@zoq I have committed your suggested changes! Thanks!

[zoq]

Thanks for working on this one, no more comments from my side.

[mlpack-bot]

Second approval provided automatically after 24 hours. 👍

[rcurtin]

@rxng8 thank you so much for working on all these issues! It's great to have them all fixed. I took a look through and I left a few comments, but really only one needs to be addressed before merge---it seems there is a copy/move constructor pair that don't have the if (this != &other) guard. Let me know if I overlooked something. :)

[rcurtin]

I didn't know randn() returned the matrix so that it could be used like this. Nice!

[rxng8]

thank you!

[zoq]

Same here, actually I had to test it out.

[rxng8]

Yes, I also tested it and it seems to return a matrix. did it happen to behave the same ways?

[rxng8]

I believe it is :))

[rcurtin]

Yep, looks great, thank you!

[rcurtin]

Thanks so much @rxng8! It seems like there are still two issues in the big checklist you made at the top, but, you could always open another PR for those two (it should be easy :)). 👍