-
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
1075 Let persons die from hospital (non ICU) in the ABM #1100
base: main
Are you sure you want to change the base?
1075 Let persons die from hospital (non ICU) in the ABM #1100
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1100 +/- ##
==========================================
- Coverage 96.98% 96.96% -0.03%
==========================================
Files 142 142
Lines 11874 11889 +15
==========================================
+ Hits 11516 11528 +12
- Misses 358 361 +3 ☔ View full report in Codecov by Sentry. |
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, not much to add, only a couple of suggestions.
To improve the codeline coverage, you could add a test where the other option in draw_backwards is checked. |
cpp/tests/test_abm_model.cpp
Outdated
// Make sure all persons will be InfectedSevere -> InfectedCritical -> Dead. | ||
ScopedMockDistribution<testing::StrictMock<MockDistribution<mio::UniformDistribution<double>>>> | ||
mock_uniform_dist; | ||
EXPECT_CALL(mock_uniform_dist.get_mock(), invoke).WillRepeatedly(testing::Return(1.0)); |
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.
Then this should be called Exactly(n_persons * 3) times. Can you test and add this (or change the description)?
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.
It is called more often than that: 5 times in drawing the viral load, 1 time per person in the infection draw_backwards, so that the persons came from Critical to Dead, four times in each person for the random_workgroups etc. and maybe more times in the evolve functions. Perhaps we can change the mocking to only affect calls in the infection draw_backwards? That would also help at other places.
There are also other instances in this test file one could change it. Not really part of this issue, but what do you think we should do? This seems to be part of #1067 .
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.
Perhaps we can change the mocking to only affect calls in the infection draw_backwards?
That is quite difficult to do, it would probably be easier (and a cleaner solution) to write the test such that only draw_backwards uses RNG in the first place.
This seems to be part of 1067 .
I am fine with adding the names of the other tests (with the necessary context) to that issue. We do not need to fix everything here, but at least we have to write down what needs fixing.
Changes and Information
Please briefly list the changes (main added features, changed items, or corrected bugs) made:
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
Every addressed issue is linked (use the "Closes #ISSUE" keyword below)
New code adheres to coding guidelines
No large data files have been added (files should in sum not exceed 100 KB, avoid PDFs, Word docs, etc.)
Tests are added for new functionality and a local test run was successful (with and without OpenMP)
Appropriate documentation for new functionality has been added (Doxygen in the code and Markdown files if necessary)
Proper attention to licenses, especially no new third-party software with conflicting license has been added
(For ABM development) Checked benchmark results and ran and posted a local test above from before and after development to ensure performance is monitored.
Branch Main:
Run on (8 X 24.0805 MHz CPU s)
CPU Caches:
L1 Data 64 KiB (x8)
L1 Instruction 128 KiB (x8)
L2 Unified 4096 KiB (x2)
Load Average: 4.99, 5.66, 5.30
Benchmark Time CPU Iterations
abm_benchmark/abm_benchmark_50k 1377 ms 1359 ms 1
abm_benchmark/abm_benchmark_100k 2748 ms 2721 ms 1
abm_benchmark/abm_benchmark_200k 5580 ms 5519 ms 1
Run on (8 X 24.0875 MHz CPU s)
CPU Caches:
L1 Data 64 KiB (x8)
L1 Instruction 128 KiB (x8)
L2 Unified 4096 KiB (x2)
Load Average: 4.66, 4.20, 4.72
Benchmark Time CPU Iterations
abm_benchmark/abm_benchmark_50k 1330 ms 1324 ms 1
abm_benchmark/abm_benchmark_100k 3187 ms 2794 ms 1
abm_benchmark/abm_benchmark_200k 5882 ms 5484 ms 1
Checks by code reviewer(s)
Closes #1075