Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1040 +/- ##
==========================================
+ Coverage 96.42% 96.54% +0.11%
==========================================
Files 132 135 +3
Lines 11061 10906 -155
==========================================
- Hits 10666 10529 -137
+ Misses 395 377 -18
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
…geted Location even without wearing mask
…sociated bool funtions/variables
reneSchm
left a comment
There was a problem hiding this comment.
I have not yet looked at the tests, but here is most of the review,
…some in the tests
|
I just noticed that the compliance vector looks very much like a parameter, especially with the getter/setter. We could think about putting it into a Person specific ParameterSet, as well as probably m_age or the random-group/hour variables; but probably in another PR. |
DavidKerkmann
left a comment
There was a problem hiding this comment.
Thanks, I have a lot of small comments and only one big one regarding the mask wearing logic. Perhaps you can give some comments.
Also, I can't remember exactly: What did we discuss in terms of checking/not checking from the location side? Right now the measures are enforced always when active. Didn't we think about an option to activate/deactivate the checks at the location?
DavidKerkmann
left a comment
There was a problem hiding this comment.
This look good for me. I only have one more small question that is still open.
|
For your comment @DavidKerkmann (#1040 (comment)): |
DavidKerkmann
left a comment
There was a problem hiding this comment.
Only some renaming and the open issue about the compliance in combination with several tests is open until this can be merged. I suppose we wait until #866 is merged to use the TestResult feature.
DavidKerkmann
left a comment
There was a problem hiding this comment.
From my side no new things, just the things left from Renes review that I didn't all check. Especially the bug in the compliance with isolation and repeated testing needs to be fixed now that the test certificates have been merged to main.
reneSchm
left a comment
There was a problem hiding this comment.
Two changes two test comments and (maybe?) a reformat, other than that I think this looks good.
reneSchm
left a comment
There was a problem hiding this comment.
With the new additions this looks good to me.
|
@reneSchm @DavidKerkmann pls always check (and "check") reviewer boxes above. |
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
Run on (8 X 24.121 MHz CPU s)
CPU Caches:
L1 Data 64 KiB (x8)
L1 Instruction 128 KiB (x8)
L2 Unified 4096 KiB (x2)
Load Average: 2.98, 4.30, 4.88
Benchmark Time CPU Iterations
abm_benchmark/abm_benchmark_50k 2247 ms 2242 ms 1
abm_benchmark/abm_benchmark_100k 4691 ms 4619 ms 1
abm_benchmark/abm_benchmark_200k 8577 ms 8299 ms 1
Benchmark Time CPU Iterations
abm_benchmark/abm_benchmark_50k 2044 ms 2040 ms 1
abm_benchmark/abm_benchmark_100k 4169 ms 4098 ms 1
abm_benchmark/abm_benchmark_200k 8243 ms 8168 ms 1
Checks by code reviewer(s)
Closes #1039
Closes #503