-
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
1039 Add functions for the Person to choose whether to comply to mask,test and isolation #1040
1039 Add functions for the Person to choose whether to comply to mask,test and isolation #1040
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
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.
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. |
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, 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?
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.
This look good for me. I only have one more small question that is still open.
For your comment @DavidKerkmann (#1040 (comment)): |
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.
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.
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.
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.
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.
Two changes two test comments and (maybe?) a reformat, other than that I think this looks good.
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.
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