-
Notifications
You must be signed in to change notification settings - Fork 7
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
HealthSystem. Convert Excel files to CSV #1527
base: master
Are you sure you want to change the base?
Conversation
Thanks @mnjowe This looks good to me, and the checks are passing --- so I'm sure all good. I'm asking @marghe-molaro to review though, as these files were introduced by her, and it would be great for he to cast her eye over it too. |
Hi @mnjowe, from a quick look it all looks in order, and if the tests are passing it should all be fine! Only thing I've noticed is that in the priority policy sheets all variables should be integers, but in the new files all those related to the fast-tracking of individuals appear to have a decimal place. Might be safer to turn them back to integers, mainly to ensure that this doesn't confuse modellers interested in adding a new policy (they might get a sense that these can be assigned non-integer value, which would however be rounded up). What do you think? Many thanks! |
Thanks @marghe-molaro. Not sure how it came to that but I agree with you that it should be integers so as it was in excel file. I will look into it |
Hi @marghe-molaro . So it turns out the floats are as a result of null values(empty rows) in the excel file columns(see attached). How do you want me to fill these columns? |
Hi @mnjowe, thanks for spotting this. Those values are never considered (see how lowest_priority_considered is loaded in the load_priority_policy fnc in the healthsystem module) so I think you can go ahead and make those "-1"s, for safety. Many thanks! |
great, thanks! |
…v_Tim_H_modules' into mnjowe/convert_excel_files_to_csv_Tim_H_modules
@marghe-molaro -- is then 'Approved' for merging then? |
This is now showing conflict! |
Let me look into it |
…dules # Conflicts: # resources/healthsystem/human_resources/scaling_capabilities/ResourceFile_dynamic_HR_scaling.xlsx
Another conflict (sorry), but as soon as this is resolved, let's merge immediately. |
Resolving it now |
…dules # Conflicts: # resources/healthsystem/priority_policies/ResourceFile_PriorityRanking_ALLPOLICIES.xlsx
@tbhallett can you please try running the failing test( |
Yes, working on my machine too. Maybe a network issue with Azure!?!? I'll restart the tests |
Same thing again! -- very weird. Let's ask @tamuri and @matt-graham, why these tests all pass locally but not on Azure. (I see it's in |
Yes, it appears so. I ran the test 20 times, and it failed twice. |
Hm. @tbhallett, could this be as a result of the new changes in CSV files? @tamuri did you also try running the master version of this test? I can try running it if you didn't already |
It's failing intermittently on master too. I suggest we continue merging the csv PRs and raise a new issue for this failing test. |
I think this non-determinism has probably been around for a while as I suspect it's what caused the non-reproducible failure of this same test mentioned in #1507 (comment). I'll see if I can do a git bisect with a longer / more sensitive version of the failing test to narrow down. |
This PR seeks to convert all excel files in healthsystem resource folder to CSV equivalent.