Fix bug with marker location prediction - #495
Conversation
f937fc4 to
f92f870
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #495 +/- ##
=======================================
Coverage 73.51% 73.51%
=======================================
Files 451 451
Lines 37692 37692
Branches 5183 5183
=======================================
Hits 27708 27708
Misses 8845 8845
Partials 1139 1139 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Thanks for catching these, Peter. This is why I much prefer porting C# to python than python to C# - sorry about that! Regarding the negative hypotheses, I found Isaac's notes and he did experiment with hypotheses like [-2,-1,0,1,2] but found that [0,1,2] gave him the best results. I assume these fixes didn't improve the marker placement in your problem project, right? |
Enkidu93
left a comment
There was a problem hiding this comment.
@Enkidu93 reviewed all commit messages and made 2 comments.
Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on pmachapman).
src/SIL.Machine/Corpora/PlaceMarkersUsfmUpdateBlockHandler.cs line 368 at r1 (raw file):
int[] hypotheses = { 0, 1, 2 }; int bestHypothesis = -1; int bestNumCrossings = 40000; // A large number
Maybe 40_000 just for slightly better readability?
f92f870 to
ddfe7ec
Compare
pmachapman
left a comment
There was a problem hiding this comment.
I assume these fixes didn't improve the marker placement in your problem project, right?
The improved for some cases, and made others far worse (particularly one of the unit tests). My original PR had a -1 case, which I removed due to one of the tests cases failing spectacularly (the markers were thrown into the preceding verse).
@pmachapman made 1 comment and resolved 1 discussion.
Reviewable status: 0 of 1 files reviewed, all discussions resolved (waiting on Enkidu93).
ddfe7ec to
12dedfb
Compare
12dedfb to
7185d27
Compare
This PR fixes a minor bug noticed in PredictMarkerLocation() when investigating token alignment issues on the current build of Serval QA.
The previous logic of
200 ^ 2meant200 XOR 2, i.e. 202.There is no -1 element in a list - I assume the last item was intended (see the Python implementation). That said this code isn't even called, as there is no hypothesis -1.
This change is