probability
probability copied to clipboard
tensorflow
New distribution: Skew-generalized Normal distribution
This pull request adds a new distribution, the generalized normal distribution variant 2, as defined on Wikipedia: https://en.wikipedia.org/wiki/Generalized_normal_distribution
This variant generalizes the Gaussian with a new parameter, the peak or shape, which controls skew and with mean and variance define an upper bound on the distribution. The classic Gaussian is the special case where this parameter is set to 0.
Thanks for putting this together! Still need to take a closer look, but in the mean time can you make the linter happy? (See TravisCI results here)
Hey Christopher,
Sorry, but this actually my very first pull request ever: I haven't ever been in a project where they were used.
I'm not sure what the linter is😰
On Wed, Jun 5, 2019 at 3:40 PM Christopher Suter [email protected] wrote:
Thanks for putting this together! Still need to take a closer look, but in the mean time can you make the linter happy? (See TravisCI results here https://travis-ci.org/tensorflow/probability/builds/540560791?utm_source=github_status&utm_medium=notification )
— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/tensorflow/probability/pull/445?email_source=notifications&email_token=AEXGFWJIX2ZHDRMVUZTTNODPZA6ENA5CNFSM4HSEI4Y2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODXBHBHQ#issuecomment-499282078, or mute the thread https://github.com/notifications/unsubscribe-auth/AEXGFWLJRZDX73RWR7ECGCLPZA6ENANCNFSM4HSEI4YQ .
-- Sincerely, Daniel Maryanovsky
No worries! Thanks for pitching in, and congrats on your first PR :)
Linters in this context perform static analysis of code changes to ensure various style guide constraints are met. Things like length of lines, indentation and inter-line whitespace. If you follow the "Details" link above, under the headings "Some checks were not successful" > "continuous-integration/travis-ci/pr", you'll find that it has a series of automated tests that were kicked off when the PR came in. The linter runs first, and if it fails nothing else runs. Once it passes, then the automated unit tests can start.
Let me know if this is sufficient info to help you proceed. The lint errors should be pretty prescriptive as far as what needs to be changed.
Thanks a lot, I'll get on it today.
On Fri, Jun 7, 2019 at 12:27 PM Christopher Suter [email protected] wrote:
No worries! Thanks for pitching in, and congrats on your first PR :)
Linters in this context perform static analysis of code changes to ensure various style guide constraints are met. Things like length of lines, indentation and inter-line whitespace. If you follow the link above, under the heading "Some checks were not successful", "continuous-integration/travis-ci/pr", you'll find that it has a series of automated tests that were kicked off when the PR came in. The linter runs first, and if it fails nothing else runs. Once it passes, then the automated unit tests can start.
Let me know if this is sufficient info to help you proceed. The lint errors should be pretty prescriptive as far as what needs to be changed.
— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/tensorflow/probability/pull/445?email_source=notifications&email_token=AEXGFWNFA3C777JMF5SH333PZKZBLA5CNFSM4HSEI4Y2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODXGYYFY#issuecomment-500010007, or mute the thread https://github.com/notifications/unsubscribe-auth/AEXGFWKZ7SYMGJIHZEYT6GTPZKZBLANCNFSM4HSEI4YQ .
-- Sincerely, Daniel Maryanovsky
Can we include this more general family of unified skewed normal distribution please?(https://www.researchgate.net/profile/Wei_Ning/publication/257459090_On_Some_Properties_of_the_Unified_Skew_Normal_Distribution/links/54f883260cf2ccffe9df4c43/On-Some-Properties-of-the-Unified-Skew-Normal-Distribution.pdf?origin=publication_detail)
Sure, in a later build though
On Thu, Jul 18, 2019 at 2:13 PM Chen Chen [email protected] wrote:
Can we include this more general family of unified skewed normal distribution please?( https://www.researchgate.net/profile/Wei_Ning/publication/257459090_On_Some_Properties_of_the_Unified_Skew_Normal_Distribution/links/54f883260cf2ccffe9df4c43/On-Some-Properties-of-the-Unified-Skew-Normal-Distribution.pdf?origin=publication_detail )
— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/tensorflow/probability/pull/445?email_source=notifications&email_token=AEXGFWK3EUEWRIUW2GWRAXTQADMIRA5CNFSM4HSEI4Y2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD2JZV4Y#issuecomment-512989939, or mute the thread https://github.com/notifications/unsubscribe-auth/AEXGFWORME5242R3GGT3MNTQADMIRANCNFSM4HSEI4YQ .
-- Sincerely, Daniel Maryanovsky
No worries! Thanks for pitching in, and congrats on your first PR :)
Linters in this context perform static analysis of code changes to ensure various style guide constraints are met. Things like length of lines, indentation and inter-line whitespace. If you follow the "Details" link above, under the headings "Some checks were not successful" > "continuous-integration/travis-ci/pr", you'll find that it has a series of automated tests that were kicked off when the PR came in. The linter runs first, and if it fails nothing else runs. Once it passes, then the automated unit tests can start.
Let me know if this is sufficient info to help you proceed. The lint errors should be pretty prescriptive as far as what needs to be changed.
Hey csuter, I had a question
I got a lint error reading that I had a bad call to super. What's the style requirement for handling super?
It should be super(SkewNormal, self).foo
Curious: why do you also subclass Normal instead of just Distribution?
On Fri, Sep 6, 2019, 6:09 AM inhandan [email protected] wrote:
No worries! Thanks for pitching in, and congrats on your first PR :)
Linters in this context perform static analysis of code changes to ensure various style guide constraints are met. Things like length of lines, indentation and inter-line whitespace. If you follow the "Details" link above, under the headings "Some checks were not successful" > "continuous-integration/travis-ci/pr", you'll find that it has a series of automated tests that were kicked off when the PR came in. The linter runs first, and if it fails nothing else runs. Once it passes, then the automated unit tests can start.
Let me know if this is sufficient info to help you proceed. The lint errors should be pretty prescriptive as far as what needs to be changed.
Hey csuter, I had a question
I got a lint error reading that I had a bad call to super. What's the style requirement for handling super?
— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/tensorflow/probability/pull/445?email_source=notifications&email_token=AFJFSIZ3NQCSEZG5DT3AUVTQIIT5NA5CNFSM4HSEI4Y2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD6CMZHY#issuecomment-528796831, or mute the thread https://github.com/notifications/unsubscribe-auth/AFJFSI44L53VM6OFJC2SWX3QIIT5NANCNFSM4HSEI4YQ .
It should be super(SkewNormal, self).foo Curious: why do you also subclass Normal instead of just Distribution? … On Fri, Sep 6, 2019, 6:09 AM inhandan @.***> wrote: No worries! Thanks for pitching in, and congrats on your first PR :) Linters in this context perform static analysis of code changes to ensure various style guide constraints are met. Things like length of lines, indentation and inter-line whitespace. If you follow the "Details" link above, under the headings "Some checks were not successful" > "continuous-integration/travis-ci/pr", you'll find that it has a series of automated tests that were kicked off when the PR came in. The linter runs first, and if it fails nothing else runs. Once it passes, then the automated unit tests can start. Let me know if this is sufficient info to help you proceed. The lint errors should be pretty prescriptive as far as what needs to be changed. Hey csuter, I had a question I got a lint error reading that I had a bad call to super. What's the style requirement for handling super? — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#445?email_source=notifications&email_token=AFJFSIZ3NQCSEZG5DT3AUVTQIIT5NA5CNFSM4HSEI4Y2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD6CMZHY#issuecomment-528796831>, or mute the thread https://github.com/notifications/unsubscribe-auth/AFJFSI44L53VM6OFJC2SWX3QIIT5NANCNFSM4HSEI4YQ .
A silly reason, to leverage some methods that are already implemented in Normal. I'll just re-write the ones I need and subclass directly from Distribution. Just to be certain, any call to super() must have the most immediate inherited class as a first argument? Is this standard, or just a Google style thing?
The call to super should generally have the class in which the method is implemented as its first argument. This is pretty conventional python (though in py3 you just write super()--can't wait!). https://www.pythonforbeginners.com/super/working-python-super-function
Basically it's going to look up the method resolution order for the given class, skipping the class itself (only looking at superclasses),
I closed by accident! A force push from an old version was made to my branch, but I've since recovered the working version with all commits. Reopening now
Are you ready for us to take another look at this? Just ping when you think it's ready.
Actually, I'm a bit confused. It works fine on my machine, but we get an import error when running it in checks. I'm really at a loss as to why.
You mean that the Travis build fails? It's reasonable to assume that's unrelated to this PR. Is everything in good shape otherwise?
@inhandan You need to add a bazel BUILD target for the new file, like this: https://github.com/tensorflow/probability/blob/master/tensorflow_probability/python/distributions/BUILD#L149
Also add a dependency from "distributions" to it, here: https://github.com/tensorflow/probability/blob/master/tensorflow_probability/python/distributions/BUILD#L103
If you want to check that it's working without roundtripping to Travis, you can install bazel (https://bazel.build/) on your machine and run some test locally to check imports.
It seems like this issue is almost resolved. I would like to help resolve conflicts, update the class structure if necessary and obtain a successful build.
I’ve been incredibly busy with work and the pandemic. I just need the damn Bazel build. I’ll let you know this week if I need anything. As it stands I think I’ll be done soon
For the BUILD rules, you should be able to copy how 'gamma' does it (for example). ":gamma" is added as a dependency for init.py: https://cs.opensource.google/tensorflow/probability/+/master:tensorflow_probability/python/distributions/BUILD;drc=ccd132d97f3edca8ade43ddb9f11660f1e7d927d;l=68
And we have associated BUILD rules for "gamma": https://cs.opensource.google/tensorflow/probability/+/master:tensorflow_probability/python/distributions/BUILD;drc=ccd132d97f3edca8ade43ddb9f11660f1e7d927d;l=518
and
"gamma_test" https://cs.opensource.google/tensorflow/probability/+/master:tensorflow_probability/python/distributions/BUILD;drc=ccd132d97f3edca8ade43ddb9f11660f1e7d927d;l=2050
Let me know if there are other issues / concerns.
@csuter recently added some helper scripts to make testing easier https://github.com/tensorflow/probability/blob/master/CONTRIBUTING.md#helper-scripts
Hey all! It passed all checks. I feel so ridiculous for putting off the Bazel build for so long, it took no time. One issue, I did not built a test.py file for the new class I made. I don't know if that's necessary for a PR.
To your immediate question about tests, the answer is yes, you should add some basic tests in a corresponding test.py file. You can also get some good test coverage by adding support for the distribution to distribution_properties_test.py.
It seems like this issue is almost resolved. I would like to help resolve conflicts, update the class structure if necessary and obtain a successful build.
So I'm encumbered as hell with work still because of quarantine. How do you feel about implementing some tests in a skew_generalized_normal_test file?
I'd have to dig a little to see what the standard test methods are. I'll be working on it too, but just much less. (My startup requires 18 hour days because of booming demand during quarantine.) I can definitely answer any questions that you might have.
If the distribution has a tractable cdf I think using the DKWM bound test should suffice to validate sample and cdf consistency. Gamma has an example here. https://cs.opensource.google/tensorflow/probability/+/master:tensorflow_probability/python/distributions/gamma_test.py;l=418 To validate the pdf a simple check would be whether it is close to the gradient of the cdf.
Not yet I don’t think. I’ll finish up some planned tests today, should be ready tomorrow
On Mon, Aug 24, 2020 at 10:38 AM Brian Patton [email protected] wrote:
Is this ready to review? At a glance the tests look OK.
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/tensorflow/probability/pull/445#issuecomment-679268226, or unsubscribe https://github.com/notifications/unsubscribe-auth/AEXGFWJ5VK3IAHWZLR4IGBLSCKQSZANCNFSM4HSEI4YQ .
-- Sincerely, Daniel Maryanovsky