-
Notifications
You must be signed in to change notification settings - Fork 56
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
Bug fix in FardalStreamDF. #358
Conversation
This is a good catch - thanks! I did have a reason for changing these values back when I implemented it, but I can't track down why. It does make sense for the class called |
Thanks for reviewing the change! It's a great idea to protect the change by a flag for backwards compatibility. But I guess you could be the better person implementing it in terms of naming, adding description and consistent code style. Just let me know. Thanks! |
Sure -- I can take it from here. I'll just contribute to this PR, so your contribution is still tracked. Thanks again for raising this! |
Thanks for the thorough fixes! |
Describe your changes
Fix the implementation of FardalStreamDF based on https://arxiv.org/pdf/1410.1861.pdf.
Specifically, we made the changes based on Eq. (21) and Eq. (6) in the paper.
Checklist
CHANGES.rst
)