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

Updates on Chi squared and Lower incomplete gamma function #122

Merged
merged 2 commits into from
Jan 24, 2024
Merged

Updates on Chi squared and Lower incomplete gamma function #122

merged 2 commits into from
Jan 24, 2024

Conversation

estebanz01
Copy link
Owner

Summary

While doing some debugging and research on why #115 was giving us different values from software like R or scipy on python, I found out two things:

  1. The chi squared calculations that were failing were caused by manually calculating the CDF for the case when DF is 2, instead of using the special case which is similar to the exponential distribution, as outlined in the wiki. When implemented, I was trying to be as general as possible, but in this scenario, it was really needed 😛
  2. The second change is related to the calculation of the lower incomplete gamma function, which was performing less iterations than desired when using the simpson's rule to calculate the integral. It was introduced in Issue 78: Solve Division by Zero when performing a chi squared good fitness test. #79 but I don't consider it a bug per se, hence the integration in this PR.

Both changes have rspec tests to cover all cases.

@estebanz01
feat: Calculate CDF for Xi square using a special case when df = 2.
df07ada 
This change introduces the calculation of the CDF for the xi square
distribution by executing the special case when the degrees of
freedom is 2. This is to make it on par with calculations made in R
and some python packages. It derived from work made by @oliver-czulo
in #115.
@estebanz01
fix: Always use at least 10_000 iterations when calculating lower gamma.
0ed5c05 
This change fixes a small bug where we were calculating the
lower incomplete gamma function via simpson's rule with lower
than 10_000 iterations, which is not ideal. This was found while
doing some debugging on #115.
@estebanz01 estebanz01 self-assigned this Jan 22, 2024
@estebanz01
Copy link
Owner Author

@oliver-czulo can you take a look at this PR ? it should unblock yours and we can merge it soon!

@estebanz01 estebanz01 mentioned this pull request Jan 22, 2024
Merged
jasoncaryallen
jasoncaryallen approved these changes Jan 23, 2024
View reviewed changes
Copy link
Contributor

@jasoncaryallen jasoncaryallen left a comment

Choose a reason for hiding this comment

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

👍 I'm not as familiar with Simpson's rule calculations, but the PR explanation and specs make sense and pass! I'd have someone more familiar give this an additional set of eyes just to be certain.

@estebanz01 estebanz01 merged commit 1876676 into master Jan 24, 2024
4 checks passed
@estebanz01 estebanz01 deleted the handle-cdf-special-case-xi branch January 24, 2024 13:44
@estebanz01 estebanz01 mentioned this pull request Jan 24, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jasoncaryallen jasoncaryallen jasoncaryallen approved these changes

Assignees

@estebanz01 estebanz01

Labels
None yet
Projects
Statistics
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@estebanz01 @jasoncaryallen