-
Notifications
You must be signed in to change notification settings - Fork 692
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
Fixes Docset for Discretize PR#742 #820
base: dev
Are you sure you want to change the base?
Conversation
@kris-singh Just fixing doc-tests will not suffice. |
@kris-singh You need to add tests for the changes you have made. |
@yashu-seth I was looking for the tests of Discretize methods i couldn't find them. Also, do you have any references for discretizing of multivariable Continuos Factors |
@kris-singh You have added the functionality to discretize univariate |
@yashu-seth Yes now discretization works for continuous factor. |
@kris-singh But only for univariate cases. So you will have to add tests for discretize method taking univariate Continuous Factors. |
@yashu-seth can you suggest some test. i was thinking about it. How can we check are we discretizing correctly or not. |
@kris-singh You can compute the values of probability at specific points using the pdf and check if you are getting the same values. |
@ankurankan @yashu-seth I have added the test.Are they enough? I am not able to understand travis-ci error how can values change when using python 3.4,.5 and python 2.7. |
@kris-singh Because there are a lot of changes in the behavior of python 2 and 3. Just look through your changes and see if you can see something that might be behaving differently in python 2 and 3. If are you unable to find something try running a debugger on both 2 and 3 and see where the values start differing. That will help you in locating the bug. |
1 similar comment
@ankurankan this is ready for review |
np.testing.assert_almost_equal(results2, [-2.120911, -0.115055, 0.198042, 0.033288, 0.004727], decimal=4) | ||
np.testing.assert_almost_equal(results3, [0.038335, 0.059015, 0.039992], decimal=4) | ||
np.testing.assert_almost_equal(results3, [0.008325, 0.028291, 0.030074], decimal=4) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kris-singh Why did these values change ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changes made to division made in python3.
for python2.7 i used from future import division
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kris-singh But that does mean that the tests were either wrong earlier or are now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think i have to figure this one out. I will check my calculations again.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ankurankan Yes i checked the calculations. Both python 2.7 and 3.5 give the ans. The only trouble was i wrote 5/2 in python 2.7 gamma(5/2) = 1 if gamma(5.0/2.0) = 1.3293. division in python3.5 are by default are float.
@ankurankan ready for review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kris-singh There is something wrong with the implementation. The discretized values shouldn't be negative as it's a probability distribution.
@ankurankan i will check the problem out. |
No description provided.