Addition of Normal Distribution to ANN module #2382

nishantkr18 · 2020-04-22T18:36:28Z

Hey!
The implementation of Normal Distribution in #1912 had a small bug in the LogProbablity() function. Also, the backward pass and tests for the class was not present.
This is an attempt to complete the work, as normal distribution is of use in many places, especially for continuous action space environments in RL module.
For the tests, I have taken 4 random values of mean, sigma and x to produce corresponding LogProbabilites and gradients, and compared it with pytorch's implementation.
I use this snippet to get the prob, dmu and dsigma.

mu = torch.tensor(1.1, requires_grad = True)
std = torch.tensor(0.1, requires_grad = True)
n = Normal(mu, std)
action_tensor = torch.tensor(1.05, requires_grad = False)
log_prob = n.log_prob(action_tensor)
ratio = torch.exp(log_prob)

print(log_prob.data.numpy())
ratio.backward()
print('gradients of mu and sigma: ', mu.grad.data.numpy(), std.grad.data.numpy())

Please have a look and share your thoughts :)

src/mlpack/tests/ann_dist_test.cpp

src/mlpack/methods/ann/dists/normal_distribution.hpp

zoq · 2020-04-25T19:47:34Z

src/mlpack/tests/ann_dist_test.cpp

@@ -127,4 +128,40 @@ BOOST_AUTO_TEST_CASE(JacobianBernoulliDistributionLogisticTest)
  }
 }

+/**
+ * Normal Distribution module test.


Should we run the Jacobian tests for the normal distribution as well?

Oh yes.. definitely! I've made the changes.. pls have a look

nishantkr18 · 2020-05-07T12:07:13Z

closed and reopened PR to re-trigger azure pipelines

kartikdutt18

Hey @nishantkr18, Thanks for working on this. I have left some minor style comments.
Other than that, I have more of a question which is related to this comment here in #1730.

Do you think that maybe we should update this PR and Bernoulli distribution in another PR and remove that section. Maybe @zoq could provide some input on this one. The only problem that I think that would be there is that a user would have to change the include statement. Maybe I missed something.

Kindly Let me know what you think.
Thanks for the contribution!

kartikdutt18 · 2020-05-07T17:23:16Z

src/mlpack/methods/ann/dists/normal_distribution.hpp

+ * Normal distribution is a function which accepts a mean and a standard deviation
+ * term and creates a probablity distribution out of it.
+ */
+template <typename DataType = arma::mat>


Hey @nishantkr18, Could also please add a template parameter description. Thanks.

kartikdutt18 · 2020-05-07T17:26:37Z

src/mlpack/methods/ann/dists/normal_distribution.hpp

+  /**
+   * Return the mean.
+   */


Hey @nishantkr18, For single line comments, do you mind using // to comment. Thanks a lot!

kartikdutt18 · 2020-05-07T17:27:47Z

src/mlpack/methods/ann/dists/normal_distribution.hpp

+  const DataType& Mean() const { return mean; }
+
+  /**
+   * Return a modifiable copy of the mean.


Suggested change

* Return a modifiable copy of the mean.

//! Modify the mean.

kartikdutt18 · 2020-05-07T17:28:13Z

src/mlpack/methods/ann/dists/normal_distribution.hpp

+  /**
+   * Return the standard deviation.
+   */


Suggested change

/**

* Return the standard deviation.

*/

//! Get the standard deviation.

kartikdutt18 · 2020-05-07T17:31:11Z

src/mlpack/methods/ann/dists/normal_distribution.hpp

+  {
+    // We just need to serialize each of the members.
+    ar & BOOST_SERIALIZATION_NVP(mean);
+    ar & BOOST_SERIALIZATION_NVP(sigma);
+  }


I am sorry, it's a picky comment, Do you mind shifting this to the implementation file. Most of the code base follows that. Thanks a lot!

nishantkr18 · 2020-05-08T18:40:25Z

I have left some minor style comments.

Thanks for those, I'll get them sorted. 👍

Do you think that maybe we should update this PR and Bernoulli distribution in another PR and remove that section.

Yeah, that sounds like a plan. Again I'm waiting for @zoq or @rcurtin 's reviews.
I think GaussianDistribution is used in a lot of places, and has API differences to NormalDistribution in ann/dist, similar is the case with Bernoulli dist. It'd be grt if we work on this in a separate PR since getting it merged might take some time. And I'd love to have a working normal-distribution just like this for now, so that we can get the #1912 PR moving. what do u say?

kartikdutt18 · 2020-05-08T18:42:50Z

And I'd love to have a working normal-distribution just like this for now, so that we can get the #1912 PR moving. what do u say?

Sure, I am fine with that. Maybe after this gets merged, we can open an issue to discuss the same, provided it needs discussing.

nishantkr18 · 2020-05-08T19:05:50Z

Maybe after this gets merged, we can open an issue to discuss the same, provided it needs discussing.

Yeah sure 👍 , sorry for the two commits, forgot to add description in the first one

src/mlpack/methods/ann/dists/normal_distribution.hpp

src/mlpack/methods/ann/dists/normal_distribution_impl.hpp

src/mlpack/tests/ann_dist_test.cpp

zoq

No further comments from my side, thanks for putting this together.

mlpack-bot

Second approval provided automatically after 24 hours. 👍

favre49

Don't have any comments, could you add to HISTORY.MD as well before we merge?

nishantkr18 · 2020-05-13T06:56:14Z

Don't have any comments, could you add to HISTORY.MD as well before we merge?

Done 👍

zoq · 2020-05-14T21:01:58Z

Thanks again for the contribution!

robotcator and others added 5 commits April 22, 2020 16:58

add normal distribution

36ca245

bug fixes in logProbablity

8151d50

added backward function in normal dist

ec750e0

doc changes and tabs removal

6cf26e0

added test for normal-dist

6718255

mlpack-bot bot added s: needs review s: unanswered s: unlabeled labels Apr 22, 2020

style bug fix

144f573

zoq added c: methods t: added feature and removed s: unanswered s: unlabeled labels Apr 22, 2020

zoq reviewed Apr 23, 2020

View reviewed changes

added suggested changes

2353d10

nishantkr18 requested a review from zoq April 25, 2020 19:00

zoq reviewed Apr 25, 2020

View reviewed changes

added jacobian test and added template to normal dist

12b68e7

nishantkr18 force-pushed the normal-dist branch from 127a61c to 12b68e7 Compare April 26, 2020 20:04

nishantkr18 requested a review from zoq April 27, 2020 05:05

birm self-requested a review April 29, 2020 23:54

nishantkr18 closed this May 7, 2020

nishantkr18 reopened this May 7, 2020

kartikdutt18 reviewed May 7, 2020

View reviewed changes

comment changes

90c6177

added template parameter discription

6e0973d

zoq reviewed May 8, 2020

View reviewed changes

nishantkr18 added 2 commits May 9, 2020 02:37

suggested changes added

6298b62

added .vscode by mistake..

db78bbb

nishantkr18 requested a review from zoq May 10, 2020 19:27

zoq approved these changes May 11, 2020

View reviewed changes

mlpack-bot bot approved these changes May 12, 2020

View reviewed changes

mlpack-bot bot removed the s: needs review label May 12, 2020

favre49 approved these changes May 13, 2020

View reviewed changes

Update HISTORY.md

4293c98

zoq merged commit 84c08fe into mlpack:master May 14, 2020

nishantkr18 deleted the normal-dist branch December 26, 2020 06:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Addition of Normal Distribution to ANN module #2382

Addition of Normal Distribution to ANN module #2382

nishantkr18 commented Apr 22, 2020 •

edited

zoq Apr 25, 2020

nishantkr18 Apr 26, 2020

nishantkr18 commented May 7, 2020

kartikdutt18 left a comment

kartikdutt18 May 7, 2020

kartikdutt18 May 7, 2020

kartikdutt18 May 7, 2020

kartikdutt18 May 7, 2020

kartikdutt18 May 7, 2020

nishantkr18 commented May 8, 2020

kartikdutt18 commented May 8, 2020

nishantkr18 commented May 8, 2020 •

edited

zoq left a comment

mlpack-bot bot left a comment

favre49 left a comment

nishantkr18 commented May 13, 2020

zoq commented May 14, 2020

Addition of Normal Distribution to ANN module #2382

Addition of Normal Distribution to ANN module #2382

Conversation

nishantkr18 commented Apr 22, 2020 • edited

zoq Apr 25, 2020

Choose a reason for hiding this comment

nishantkr18 Apr 26, 2020

Choose a reason for hiding this comment

nishantkr18 commented May 7, 2020

kartikdutt18 left a comment

Choose a reason for hiding this comment

kartikdutt18 May 7, 2020

Choose a reason for hiding this comment

kartikdutt18 May 7, 2020

Choose a reason for hiding this comment

kartikdutt18 May 7, 2020

Choose a reason for hiding this comment

kartikdutt18 May 7, 2020

Choose a reason for hiding this comment

kartikdutt18 May 7, 2020

Choose a reason for hiding this comment

nishantkr18 commented May 8, 2020

kartikdutt18 commented May 8, 2020

nishantkr18 commented May 8, 2020 • edited

zoq left a comment

Choose a reason for hiding this comment

mlpack-bot bot left a comment

Choose a reason for hiding this comment

favre49 left a comment

Choose a reason for hiding this comment

nishantkr18 commented May 13, 2020

zoq commented May 14, 2020

nishantkr18 commented Apr 22, 2020 •

edited

nishantkr18 commented May 8, 2020 •

edited