Add chapter-level data counts to execution data#990
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #990 +/- ##
==========================================
+ Coverage 65.47% 65.87% +0.39%
==========================================
Files 387 388 +1
Lines 21747 21952 +205
Branches 2770 2782 +12
==========================================
+ Hits 14238 14460 +222
+ Misses 6472 6440 -32
- Partials 1037 1052 +15 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
I guess I should add this to Echo as well. I'll add that. |
I'm realizing that I'll need a Machine dependency in Echo in order to do that. Is that OK or would you prefer to just wait until we complete #987 and let it be resolved there? |
2363b49 to
e52b0c5
Compare
|
Our conclusion was that we will only count scripture segments in this new counts data structure. |
ddaspit
left a comment
There was a problem hiding this comment.
@ddaspit reviewed 20 files and all commit messages, and made 1 comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on Enkidu93).
src/Machine/src/Serval.Machine.Shared/Models/BuildExecutionData.cs line 9 at r1 (raw file):
public bool? IsTrainFilteredByChapter { get; init; } public bool? IsInferenceFilteredByChapter { get; init; } public Dictionary<string, Dictionary<string, int>>? TrainVerseCount { get; init; }
Can we use IReadOnlyDictionary? Does it cause issues?
Enkidu93
left a comment
There was a problem hiding this comment.
@Enkidu93 made 1 comment.
Reviewable status: 7 of 21 files reviewed, 1 unresolved discussion (waiting on ddaspit).
src/Machine/src/Serval.Machine.Shared/Models/BuildExecutionData.cs line 9 at r1 (raw file):
Previously, ddaspit (Damien Daspit) wrote…
Can we use
IReadOnlyDictionary? Does it cause issues?
Done. No, I double-checked and I think we're fine to use it. It makes passing it through all the layers a little uglier is all - esp. since it is nested.
I decided to leave on Echo for now. I can add support if you prefer but hopefully we will do that work in this sprint as well anyways. |
|
Let me know what you think of my refactoring to be able to better test the preprocess row processing. |
Fixes #907
@ddaspit, my only outstanding question is how we'd like to count only pretranslated non-verse text. At the moment, they're all lumped together. We could either not count them at all or count them separately or just lump them in (current behavior). What do you think makes the most sense?
This change is