Feature/TASK 3483 risk calculation v2 #1457
Conversation
| // MARK: - Internal | ||
|
|
||
| let min: Double | ||
| let max: Double | ||
|
|
||
| let minExclusive: Bool? | ||
| let maxExclusive: Bool? |
There was a problem hiding this comment.
Thats not internal but quite public ;)
Could you add comments on these properties. Are minExclusive and maxExclusive set externally?
There was a problem hiding this comment.
Well, they were indeed internal since that is the default visibility in Swift, not public. But since they are not written or read externally we changed them to private now 🤓👍
nickguendling
changed the title
|
|
||
| import Foundation | ||
|
|
||
| struct CWARange: Decodable { |
There was a problem hiding this comment.
Do you have automated tests specifically for CWARange?
src/xcode/ENA/ENA/Source/Services/Risk/CalculationV2/Models/CWARiskLevel.swift
Show resolved
Hide resolved
src/xcode/ENA/ENA/Source/Services/Risk/CalculationV2/Models/ExposureDetectionResult.swift
Outdated
Show resolved
Hide resolved
| var ageInDaysOfMostRecentDateWithLowRisk: Int? { | ||
| guard let mostRecentDateWithLowRisk = mostRecentDateWithLowRisk else { | ||
| return nil | ||
| } | ||
|
|
||
| return Calendar.current.dateComponents([.day], from: mostRecentDateWithLowRisk, to: Date()).day | ||
| } | ||
|
|
||
| var ageInDaysOfMostRecentDateWithHighRisk: Int? { | ||
| guard let mostRecentDateWithHighRisk = mostRecentDateWithHighRisk else { | ||
| return nil | ||
| } | ||
|
|
||
| return Calendar.current.dateComponents([.day], from: mostRecentDateWithHighRisk, to: Date()).day | ||
| } |
There was a problem hiding this comment.
These two are only used for the unit tests to make them independent of specific dates, right? Can we move this into the test coding?
src/xcode/ENA/ENA/Source/Services/Risk/CalculationV2/Models/ExposureWindow.swift
Outdated
Show resolved
Hide resolved
|
|
||
| import Foundation | ||
|
|
||
| final class RiskCalculationV2 { |
There was a problem hiding this comment.
Not entirely sure how namespaces work in Swift, but does this have to have V2 in its name, it the folder has it already?
There was a problem hiding this comment.
We don't care about folders 🤡 So yes, having a dedicated V2 helps – unless we use Swift Packages…
There was a problem hiding this comment.
We have to keep V2 naming for now, will rename it once we have removed the old implementation.
There was a problem hiding this comment.
Thanks for the clarification. If it was for me, you can also keep it permanently, even after the merge.
src/xcode/ENA/ENA/Source/Services/Risk/CalculationV2/RiskCalculationWindow.swift
Outdated
Show resolved
Hide resolved
|
|
||
| /// Determines the risk level for one exposure window | ||
| /// https://github.com/corona-warn-app/cwa-app-tech-spec/blob/7779cabcff42afb437f743f1d9e35592ef989c52/docs/spec/exposure-windows.md#determine-risk-level-for-exposure-windows | ||
| final class RiskCalculationWindow { |
There was a problem hiding this comment.
I'm not overly happy with the name of this. I would suggest to avoid using the term Window outside of ExposureWindow unless really necessary. How about ExposureWindowRiskAssessment? Or ExposureWindowRiskWrapper?
There was a problem hiding this comment.
In the caller context the window naming feels the most logical to us. If we rename this, we would have filteredExposureWindowRiskAssessments and exposureWindowsRiskAssessmentsPerDate or something similar.
There was a problem hiding this comment.
Well, but then you should also have e.g.filteredRiskCalculationWindows. Would ExposureWindowRiskWrapper or ExposureWindowCalculationWrapper be so bad? If it's "just a wrapper", I think calling variables filteredExposureWindows is still fine.
There was a problem hiding this comment.
We could go the other direction and make it an exposure window using the decorator pattern. We gave it a try and called it RiskCalculationExposureWindow and think this accurately describes the exposure window that is extended with risk calculation features. What do you think about this approach?
src/xcode/ENA/ENA/Source/Services/Risk/CalculationV2/RiskCalculationV2.swift
Outdated
Show resolved
Hide resolved
… private extension
This reverts commit e8ba2c7.
Description
This implements the pure new risk calculation based on exposure windows including unit tests that are needed test date from a json file. In addition, the exposure notification framework version is set to 2 and the deployment target is set to 13.7 in preparation for the use of exposure windows. The generated protobuf model will regenerated and put into the correct location in the next pull request.
Tech Spec
https://github.com/corona-warn-app/cwa-app-tech-spec/blob/7779cabcff42afb437f743f1d9e35592ef989c52/docs/spec/exposure-windows.md
Jira
https://jira-ibs.wbs.net.sap/browse/EXPOSUREAPP-2582