Skip to content
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

Model ensembles #231

Open
wants to merge 11 commits into
base: master
Choose a base branch
from
Open

Model ensembles #231

wants to merge 11 commits into from

Conversation

JamiesonWarner
Copy link
Collaborator

A couple of minor changes.

ofrancon
ofrancon previously approved these changes Sep 22, 2023
Copy link
Collaborator

@ofrancon ofrancon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

ofrancon
ofrancon previously approved these changes Oct 20, 2023
Copy link
Collaborator

@ofrancon ofrancon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm but if you have a chance please add docstrings to the function and its unit test.

@@ -69,6 +71,30 @@ def test_create_prediction_initial_context_and_action_vectors(self):
self.assertEqual(last_day_of_training_cases_data, prediction_context[-1])
gc.collect()

def test_convert_to_new_cases(self):
df = oxford_data.prepare_cases_dataframe(DATA_PATH)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 for the unit test 👍 . A docstring explaining what it does would make it even better. I had to read the code to see it was converting smooth cases per 100k back to the original new cases.

@@ -348,8 +348,18 @@ def convert_smooth_cases_per_100K_to_new_cases(smooth_cases_per_100K,
window_size,
prev_new_cases_list,
pop_size):
return (((window_size * pop_size) / 100000.) * smooth_cases_per_100K \
- np.sum(prev_new_cases_list[-(window_size-1):])).clip(min=0.0)
# Smooth cases per 100K -> Smooth cases
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function could use a docstring. It's pretty hard to understand what it does from the code...
I'm glad you have a unit test for it because I can't easily tell from the code if the logic is correct or off by 1...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants