-
Notifications
You must be signed in to change notification settings - Fork 1
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
Rdrp 1144 g weights func #422
Conversation
Is there a need for the "TestCalcWeightWithMissingVals" that is failing as the "TestCalcWeightFactor:" contains missing values? |
# Calculate 'g' for this group | ||
if (e - s) > 0: | ||
g_weight = (E - s) / (a * (e - s)) | ||
else: |
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.
are we sure a is always > 0 ?
any changes (E-s) or (e-s) can become negative?
@@ -46,7 +46,7 @@ def create_input_df(self): | |||
def create_expected_output(self): | |||
"""Create expected output boolean series for test""" | |||
expected_output = pd.Series( | |||
[True, True, True, True, False, False, False, True, True, True, True, True] | |||
[True, True, False, True, False, False, False, True, False, False, True, True] |
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.
you don't have a case where instance is 0, selectiontype is P, but 709 is null.
No big deal, but just wanted to flag as a possible extra case to test
[5, 10, 0, 0, 1.0], | ||
[1, 20, 4, 1, 5000, 161, 66, 6.3, 8.20], | ||
[2, 4, 1, 0, 5000, 77, 0, 4.0, 16.23], | ||
[4, 3, 1, 0, 5000, 88, 0, 3.0, 18.93], |
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 am a bit puzzled about why only those entries apper in this qa dataset!
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.
Now I actually understand this. Forget this comment
[7, 1, 10, "P", "Clear", "0006", 5, 10, 5000, 7, False, 1.0, 1.0], | ||
[8, 1 , np.nan, "P", "Clear", "0006", 2, 4, 5000, 7, False, 1.0, 1.0], | ||
[9, 0, 5, "P", "Clear", "0006", 1, 20, 5000, 44, False, 6.3, 8.2], | ||
[10, 0, 10, "P", "Clear", "0006", 1, 20, 5000, 44, False, 6.3, 8.2], |
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.
you don't expect me to recalculate the weights as a reviewer do you? :-D
the test is checking the right thing in the right way, but i'm not doing a test of the test by explicitly verifying these g weights are correct
Pull Request submission
Insert detailed bullet points about your changes here!
Insert any instructions to help the reviewer, e.g. "install new requirements from
requirements.txt
"*Let the reviewer know what data files are needed (and if applicable, where they are to be found)
Closes or fixes
Closes #
Code
Documentation
Any new code includes all the following forms of documentation:
Args
andreturns
for all major functionsData
Testing
Peer Review Section
requirements.txt
Final approval (post-review)
The author has responded to my review and made changes to my satisfaction.
Review comments
Insert detailed comments here!
These might include, but not exclusively:
that it is likely to interact with?)
works correctly?)
Your suggestions should be tailored to the code that you are reviewing.
Be critical and clear, but not mean. Ask questions and set actions.