add low book confidence checker

[mshannon-sil]

This replicates the logic in machine.py for checking whether a book's confidence is unusually low. It also ports the tests over.

One thing to note is it does raise an error for invalid input. I don't think this should happen, but if you think for some reason there may be scenarios where the book confidence is not between 0 and 1, Serval would need to either check for that beforehand or catch the exception, or else we can revise the function here to not throw one and return null instead. I'd hate for this to crash users' builds unexpectedly.

---

This change is 

[pmachapman]

@pmachapman reviewed 2 files and all commit messages, and made 2 comments.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on Enkidu93 and mshannon-sil).

---

src/SIL.Machine/QualityEstimation/BookConfidence.cs line 7 at r1 (raw file):

    public static class BookConfidence
    {
        public const double LowBookConfidenceThreshold = 0.42;

Can you please make this a static readonly instead of a const, as there is a chance it may change in future?

public static readonly double LowBookConfidenceThreshold = 0.42;

(A const is evaluated at compile time, a static readonly is evaluated at runtime. Making this a const could cause inconsistencies between assemblies that reference this value and the IsBookConfidenceUnusuallyLow function below if they update their reference to the library but are not themselves recompiled.)

Code quote:

public const double LowBookConfidenceThreshold = 0.42;

---

src/SIL.Machine/QualityEstimation/BookConfidence.cs line 9 at r1 (raw file):

        public const double LowBookConfidenceThreshold = 0.42;

        public static bool IsBookConfidenceUnusuallyLow(double confidence, string bookId = null, string model = null)

Can you please create a codedoc comment that documents the behavior of this function, as I think the ArgumentOutOfRangeException may be non-obvious to someone implementing a call to this function in the future?

/// <summary>
/// 
/// </summary>
/// <param name="confidence"></param>
/// <param name="bookId"></param>
/// <param name="model"></param>
/// <returns></returns>
/// <exception cref="ArgumentOutOfRangeException"></exception>

Code quote:

public static bool IsBookConfidenceUnusuallyLow(double confidence, string bookId = null, string model = null)

[Enkidu93]

@Enkidu93 reviewed 2 files and all commit messages, and made 1 comment.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on mshannon-sil and pmachapman).

---

src/SIL.Machine/QualityEstimation/BookConfidence.cs line 9 at r1 (raw file):

Previously, pmachapman (Peter Chapman) wrote…

Can you please create a codedoc comment that documents the behavior of this function, as I think the ArgumentOutOfRangeException may be non-obvious to someone implementing a call to this function in the future?

/// <summary>
/// 
/// </summary>
/// <param name="confidence"></param>
/// <param name="bookId"></param>
/// <param name="model"></param>
/// <returns></returns>
/// <exception cref="ArgumentOutOfRangeException"></exception>

Yeah, I agree. It would be reasonable for the geometric mean function to throw an error like this, but it feels a little out-of-place in this function. We could throw an exception as needed in Serval while calculating the geometric mean. I'm fine with either leaving this or just removing the exception-throwing altogether. If we do keep this exception, maybe change 'It is...' to 'It should be...'. I'm not sure: This exception should be about this function not making sense for values outside of 0-1, not about how the geometric mean requires non-negative values. This all makes me think that the QualityEstimation code itself should be calculating the mean 🤔.