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 🤔.

[mshannon-sil]

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

Previously, Enkidu93 (Eli C. Lowry) wrote…

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 🤔.

Yeah let me change the exception text to not mention the geometric mean, and just focus on it needing to be between 0 and 1.

When you say you think that the quality estimation code itself should be calculating the mean, can you clarify what you're thinking there? Is the suggested workflow that Serval might pass the segment-level confidence to a method in Machine's QualityEstimation, which would calculate the book confidence and also return the boolean?

[mshannon-sil]

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

---

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

Previously, pmachapman (Peter Chapman) wrote…

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.)

Done.

---

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

Previously, mshannon-sil wrote…

Yeah let me change the exception text to not mention the geometric mean, and just focus on it needing to be between 0 and 1.

When you say you think that the quality estimation code itself should be calculating the mean, can you clarify what you're thinking there? Is the suggested workflow that Serval might pass the segment-level confidence to a method in Machine's QualityEstimation, which would calculate the book confidence and also return the boolean?

I simplified the exception message substantially. If you still think that we shouldn't raise an exception at all, I can remove it.

[codecov-commenter]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 73.31%. Comparing base (ba0e245) to head (57f7ad0).

Additional details and impacted files

@@           Coverage Diff           @@
##           master     #479   +/-   ##
=======================================
  Coverage   73.30%   73.31%           
=======================================
  Files         445      446    +1     
  Lines       37323    37334   +11     
  Branches     5120     5121    +1     
=======================================
+ Hits        27360    27371   +11     
  Misses       8836     8836           
  Partials     1127     1127           

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[pmachapman]

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

[Enkidu93]

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

---

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

Previously, mshannon-sil wrote…

I simplified the exception message substantially. If you still think that we shouldn't raise an exception at all, I can remove it.

Great, thank you!

Well, it seems like if we want silnlp and Serval to be aligned more fully, it would make sense to move this whole process to a shared utility since there are a number of different steps at which they could diverge (e.g., which segments are filtered out, how the mean is taken, etc.). For now, this is good, but I think we should consider refactoring it in the future.

[mshannon-sil]

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

Previously, Enkidu93 (Eli C. Lowry) wrote…

Great, thank you!

Well, it seems like if we want silnlp and Serval to be aligned more fully, it would make sense to move this whole process to a shared utility since there are a number of different steps at which they could diverge (e.g., which segments are filtered out, how the mean is taken, etc.). For now, this is good, but I think we should consider refactoring it in the future.

Yeah, that makes sense!