-
Notifications
You must be signed in to change notification settings - Fork 114
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
Fix Risk Index calculation and add data sanitization #62
Conversation
547b227
to
d83fce6
Compare
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.
@yihuicai Would you mind adding a test for report.py? I know the code in report.py is really crappy, so it would be nice to have a test for it. Thank you for fixing the compatibility issue BTW! Really appreciate it.
Sure, just added a test for risk index report. |
7fe8168
to
f454e16
Compare
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.
Thank you for adding the unit test! I will approve this change after you fix the minor naming and parameter default issues. Thanks 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.
Thank you. LGTM.
457aa22
to
d798849
Compare
Seems the workflow didn't pass the test I wrote but did pass locally for me. I changed the accuracy of float number results. Let's see if it works. |
chunk_BG
should be 20 to calculateri_per_hour
, since the gap between two records is 3 mins.chunk_BG
may not be full. This will affect the accuracy of theri_mean
.np.mean()
is no longer compatible (see Risk index calculation error in report generation #61 ) Use similar caculation inrisk.py