-
Notifications
You must be signed in to change notification settings - Fork 16
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
1028 Reduce export_input_data_county_timeseries #1029
1028 Reduce export_input_data_county_timeseries #1029
Conversation
…urns-different-values-than-read_input_data_county
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1029 +/- ##
========================================
Coverage 96.44% 96.45%
========================================
Files 135 135
Lines 11286 10817 -469
========================================
- Hits 10885 10433 -452
+ Misses 401 384 -17 ☔ View full report in Codecov by Sentry. |
I have compared the runtime of the new extrapolation function with the old one. The results are almost identical. The result shown are for the secir model.
|
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 your work!
A review of the tests is actually still missing.
Small explanation for the rather detailed review of the parameters_io files:
I wanted to write similar functionality for another model and found these files quite confusing. Less code duplication, less function arguments and a complete and correct documentation would have helped. That's why I put more emphasis on this.
Please in particular consider combining the functions export_input_data_county_timeseries and read_input_data_county.
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.
Nice work :) I like the ideas of the tests. Please change some small things.
…urns-different-values-than-read_input_data_county
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 all the work and for including the review comments. I have just a few more comments (and some of them are optional).
Co-authored-by: lenaploetzke <[email protected]>
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.
Very good :)
Changes and Information
Please briefly list the changes (main added features, changed items, or corrected bugs) made:
export_input_data_county_timeseries
is similar to the functionality inread_input_data_county
.If need be, add additional information and what the reviewer should look out for in particular:
Merge Request - Guideline Checklist
Please check our git workflow. Use the draft feature if the Pull Request is not yet ready to review.
Checks by code author
Checks by code reviewer(s)