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.

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

WIP - triangular distribution #1646

Conversation

jimenofonseca
Copy link
Contributor

This PR solves issue #1578. Tests in PyMC3 need to pass first.

@springcoil
Copy link
Contributor

springcoil commented Jan 6, 2017

Where's the tests? - Adding WIP to flag this as it needs a test to be added.

@springcoil springcoil changed the title traingular distribution WIP - traingular distribution Jan 6, 2017
@springcoil springcoil added the WIP label Jan 6, 2017
tt.log(2 * (value - lower) / ((upper - lower) * (c - lower))),
tt.switch(alltrue_elemwise([value == c]), tt.log(2 / (upper - lower)),
tt.switch(alltrue_elemwise([c < value, value <= upper]),
tt.log(2 * (upper - value) / ((upper - lower) * (upper - c))), np.inf)))
Copy link
Member

Choose a reason for hiding this comment

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

New new-line.



class Triangular(Continuous):
"""
Copy link
Member

Choose a reason for hiding this comment

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

Doc-string could be improved, see the other doc-strings that include the support of the parameters as well a latex string of the math.

@jimenofonseca
Copy link
Contributor Author

jimenofonseca commented Jan 9, 2017

@springcoil sorry, i am new in helping in this repository. What are the tests that should be included? where do i find them and how do i run them?

@ColCarroll
Copy link
Member

hey @jimenofonseca -- check out pymc3/tests/test_distributions.py and pymc3/tests/test_distributions/random.py. We enforce there that pymc3 is [statistically] as good as scipy, meaning you'll compare the distribution to scipy.stats.triang. It should look a lot like the test_normal and TestNormal cases. Let me know if that's still confusing!

To run the tests, you can either use docker (see docs!), or assuming everything is where it should be, ./scripts/test.sh -vx pymc3/tests/test_distributions.py:TestMatchesScipy should also work locally.

@jimenofonseca
Copy link
Contributor Author

@ColCarroll thanks, the test runs now, I hope this is the way to use it. please check it out.

@twiecki I am not used to Latec and the structure of the PyMC3 documentation . Would you mind improving the Doc-string?

@@ -24,6 +24,7 @@
from .continuous import ExGaussian
from .continuous import VonMises
from .continuous import SkewNormal
from .continuous import Triangular
Copy link
Member

Choose a reason for hiding this comment

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

you'll need to add 'Triangular' to the __all__ list in this file as well -- should fix the test error.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done!, thank you

@twiecki
Copy link
Member

twiecki commented Jan 18, 2017

@jimenofonseca I resolved a merge conflict, make sure to pull.

@jimenofonseca jimenofonseca changed the title WIP - traingular distribution WIP - triangular distribution Jan 24, 2017
@jimenofonseca
Copy link
Contributor Author

@ColCarroll @twiecki so i guess it is ready now? :-)

@twiecki twiecki merged commit 44776a8 into pymc-devs:master Jan 24, 2017
@twiecki
Copy link
Member

twiecki commented Jan 24, 2017

thanks @jimenofonseca!

@ferrine
Copy link
Member

ferrine commented Jan 25, 2017

Hmm, maybe I'm writing too late. I see there is no transform for Triangular distribution, Is it ok?

@twiecki
Copy link
Member

twiecki commented Jan 25, 2017

@ferrine you're right, good catch.

@ferrine
Copy link
Member

ferrine commented Jan 25, 2017

Docstring can be better also

@twiecki
Copy link
Member

twiecki commented Jan 25, 2017

@jimenofonseca can you add those?

@jimenofonseca
Copy link
Contributor Author

jimenofonseca commented Jan 25, 2017 via email

@ferrine
Copy link
Member

ferrine commented Jan 25, 2017

@jimenofonseca I thing the same as for Uniform will fit there

@jimenofonseca
Copy link
Contributor Author

jimenofonseca commented Jan 26, 2017 via email

@twiecki
Copy link
Member

twiecki commented Jan 27, 2017 via email

@srihari4mbatech
Copy link

Hi,

If I upgrade Pymc3, can I get Triangular distribution, in it?

@twiecki
Copy link
Member

twiecki commented Feb 23, 2017

Yes, if you install the master.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants