Skip to content

Conversation

@eaplatanios
Copy link

This PR adds support for sampling from a Beta distribution. Are the Greek letters acceptable in the public API? If not I can replace them.

As a side note, I believe that we should ideally have a Distribution protocol that let's one sample from a distribution, but also compute other quantities like expectation, pdf, cdf, etc. For that reason, I believe a couple of interesting TODOs with respect to the Random module would be:

  • Generalize distributions over both Swift floating point types, but also TensorFlow floating point types (to allow for support of things like TensorFlow Probability).
  • Create a Distribution protocol that requires some more things for each distribution, such as computing expectation, pdf, cdf, etc.

@rxwei
Copy link
Contributor

rxwei commented Jan 13, 2019

Are the Greek letters acceptable in the public API? If not I can replace them.

Not yet, probably in the (distant) future. Replacing them sounds great.

@rxwei
Copy link
Contributor

rxwei commented Jan 13, 2019

Thanks Anthony! Will take a closer look later today.

w = a * exp(v)
break
}

Copy link
Contributor

Choose a reason for hiding this comment

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

How about removing this empty line?

/// algorithm, when both alpha and beta are greater than 1.
///
/// - Parameters:
/// - alpha: First Beta distribution shape parameter.
Copy link
Contributor

Choose a reason for hiding this comment

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

Align - with "Parameters".

Suggested change
/// - alpha: First Beta distribution shape parameter.
/// - alpha: First Beta distribution shape parameter.

@rxwei rxwei added the tensorflow This is for "tensorflow" branch PRs. label Jan 14, 2019
@rxwei
Copy link
Contributor

rxwei commented Jan 14, 2019

Our CI is down. We'll have to wait for it.

@rxwei
Copy link
Contributor

rxwei commented Jan 17, 2019

@eaplatanios I think we also need a protocol that unifies all distributions so that Tensor random initializers can take a generic distribution and just use that.

@eaplatanios
Copy link
Author

@rxwei I agree. I'll look into proposing one such protocol. I could do it as part of this PR but it will probably have to wait until after the ICML deadline (January 23rd) because I'm a bit busy until then.

It would also be nice to allow distributions to be defined over tensors (e.g., passing in a TensorFlow tensor as the mean of a distribution and returning TensorFlow tensors as samples).

@rxwei
Copy link
Contributor

rxwei commented Jan 17, 2019

@swift-ci please test tensorflow

6 similar comments
@rxwei
Copy link
Contributor

rxwei commented Jan 17, 2019

@swift-ci please test tensorflow

@rxwei
Copy link
Contributor

rxwei commented Jan 17, 2019

@swift-ci please test tensorflow

@rxwei
Copy link
Contributor

rxwei commented Jan 17, 2019

@swift-ci please test tensorflow

@rxwei
Copy link
Contributor

rxwei commented Jan 17, 2019

@swift-ci please test tensorflow

@rxwei
Copy link
Contributor

rxwei commented Jan 17, 2019

@swift-ci please test tensorflow

@rxwei
Copy link
Contributor

rxwei commented Jan 17, 2019

@swift-ci please test tensorflow

@rxwei rxwei merged commit 215b1a9 into swiftlang:tensorflow Jan 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

tensorflow This is for "tensorflow" branch PRs.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants