-
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 1098 unit tests imputation helpers #404
base: develop
Are you sure you want to change the base?
Conversation
Is it advised by Keilan Evans that the preposed changes to |
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.
Great! Look forward to seeing the other tests.
"emp_researcher", "emp_technician", "emp_other", | ||
"headcount_res_m", "headcount_res_f", "headcount_tec_m", | ||
"headcount_tec_f", "headcount_oth_x", "headcount_oth_y", | ||
"headcount_tot_x", "headcount_tot_y"] |
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 like the way you've set out the test with the expected output.
Just a suggestion, it might be easier to check that the columns match the config if they were presented in the same order, or something.
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.
thanks for the review and the advice, I'll update the code when I do the next test.
I've added the test for create_imp_class_col (I believe)... |
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.
Thanks for this next test- it looks good, and I'm happy to discuss
run2 = create_imp_class_col(run1, column_list=['201', 'rusic'], class_name='pg_class', use_cellno=True) | ||
result_df = create_imp_class_col(run2, column_list=['201'], class_name='pg_sic_class', use_cellno=True) | ||
|
||
assert_frame_equal(result_df.reset_index(drop=True), exp_output_df) |
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.
HI Rob, thanks for this new test.
We already have tests for this function, don't know if you saw those. However, you have usefully added new test cases for when we use the "pg_class" and "pg_sic_class".
If you'd like to chat some time, I'll talk you through how I would have approached this task.
RDRP-1055: TMI activated for PNP
RDRP-1094: update the backdata with extra summations
…apping RDRP 1141- adding_uni_employment_column
…hub.com/ONSdigital/research-and-development into RDRP-1098_unit_tests_imputation_helpers
Pull Request submission
The first new imputation_helper test is included: get_imputation_cols
pull request is for info as there are 10 more tests to complete
I have included a set of test data that is different to the live system
Closes or fixes
initial commit
Code
Documentation
Any new code includes all the following forms of documentation:
Args
andreturns
for all major functionsData
Testing
Review comments