Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

New distribution: Skew-generalized Normal distribution #445

Open
wants to merge 41 commits into
base: main
Choose a base branch
from
Open

New distribution: Skew-generalized Normal distribution #445

wants to merge 41 commits into from

Conversation

inhandan
Copy link

@inhandan inhandan commented Jun 3, 2019

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.

@googlebot googlebot added the cla: yes Declares that the user has signed CLA label Jun 3, 2019
@inhandan
Copy link
Author

inhandan commented Jun 3, 2019

I signed it

@csuter
Copy link
Member

csuter commented Jun 5, 2019

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)

@inhandan
Copy link
Author

inhandan commented Jun 6, 2019 via email

@csuter
Copy link
Member

csuter commented Jun 7, 2019 •
edited
Loading

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.

@inhandan
Copy link
Author

inhandan commented Jun 7, 2019 via email

@cc20002002
Copy link

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)

@inhandan
Copy link
Author

inhandan commented Jul 18, 2019 via email

@inhandan
'TF2 compat, stable. Final Mathematical checks need to be finished, b…
17ce9e7 
…ehavior for sampling, evaluating mean, mode, and stddev are confirmed.'
@inhandan
'TF2 compat, stable. Final Mathematical checks need to be finished, b…
9969fd3 
…ehavior for sampling, evaluating mean, mode, and stddev are confirmed.'
@inhandan
'Lint fixes.'
ee51cb0
@inhandan
Copy link
Author

inhandan commented Sep 6, 2019

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?

@brianwa84
Copy link
Contributor

brianwa84 commented Sep 6, 2019 via email •
edited
Loading

@inhandan
Copy link
Author

inhandan commented Sep 6, 2019

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?

@brianwa84
Copy link
Contributor

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),

@inhandan
Copy link
Author

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.

@inhandan
Copy link
Author

@brianwa84 take another look?

@brianwa84
Copy link
Contributor

brianwa84 commented Jul 21, 2020 via email

@inhandan
Copy link
Author

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.

@brianwa84
Copy link
Contributor

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.

@brianwa84
Copy link
Contributor

Is this ready to review? At a glance the tests look OK.

@inhandan
Copy link
Author

inhandan commented Aug 24, 2020 via email

@inhandan
Copy link
Author

Ready to review!

@fonnesbeck
Copy link

Is this still a viable PR? Would be great to have this distribution in TFP.

@inhandan
Copy link
Author

inhandan commented Feb 9, 2021 via email

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.

Powered by ReviewNB

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
cla: yes Declares that the user has signed CLA
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@inhandan @csuter @cc20002002 @brianwa84 @axch @kori73 @srvasude @fonnesbeck @googlebot