Add IsEquivalentTo versification extension method

[Enkidu93]

Also, only convert USFM versification when versifications are not equivalent.

Partial fix for sillsdev/serval#1043.

I'm not sure how extensively we want to test this or what all we might want to count as differences, but this seemed like a reasonable approach based on what was available to me through the versification object. Let me know what you think.

---

This change is 

[pmachapman]

@pmachapman reviewed 3 files and all commit messages, and made 1 comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on ddaspit and Enkidu93).

---

src/SIL.Machine/Corpora/ScrVersExtensions.cs line 90 at r1 (raw file):

                }
            }
            return true;

This method seems pretty intense as it loads every verse reference as a struct into memory instead of just the differences?

After looking at the Versification source code, another approach you could consider ScrVers.Save() which calls Versification.WriteToStream() to generate a string you could compare:

using (var scrVersWriter = new StringWriter())
using (var otherWriter = new StringWriter())
{
	scrVers.Save(scrVersWriter);
	other.Save(otherWriter);

	return scrVersWriter.ToString() == otherWriter.ToString();
}

In my tests, I found it used about 26.8MB of memory for the unit test as opposed to 28.2MB, although it ran slightly slower (~96ms vs ~86ms on average for your implementation).

I think the speed difference is down to differences will more often than not be hit earlier rather than later, as opposed to my example above of generating a string for comparison, which although more memory efficient, isn't really faster when the versifications are different.

Up to you - I did notice that when I modified your unit test for customVrs2 to have verse segments added:

string src = "*MAT 1:5,-,a,b,c,d,e,f\r\nMAT 1:1 = MAT 1:2\r\nMAT 1:2 = MAT 1:1\r\n";

And set when I set a breakpoint on return false; thisVerse was ESG 4:20, instead of Matthew 1:5 or Matthew 1:6 which I expected it to return false on?

Code quote:

            // If all verses in the versifications are 1) equal (accounts for mapping)
            // and 2) graphically identical in regard to book, chapter, and verse, then the versifications are equivalent
            foreach (
                (VerseRef thisVerse, VerseRef otherVerse) in scrVers
                    .AllIncludedVerses()
                    .Zip(other.AllIncludedVerses())
                    .Select(tup => (tup.Item1, tup.Item2))
            )
            {
                if (
                    !(
                        thisVerse.ChangeVersificationWithSegments(otherVerse.Versification).Equals(otherVerse)
                        && thisVerse.VerseNum == otherVerse.VerseNum
                        && thisVerse.ChapterNum == otherVerse.ChapterNum
                        && thisVerse.BookNum == otherVerse.BookNum
                    )
                )
                {
                    return false;
                }
            }
            return true;

[codecov-commenter]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 74.35%. Comparing base (c7146cd) to head (ac247f1).

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #525      +/-   ##
==========================================
+ Coverage   74.34%   74.35%   +0.01%     
==========================================
  Files         456      456              
  Lines       38261    38283      +22     
  Branches     5242     5245       +3     
==========================================
+ Hits        28445    28467      +22     
  Misses       8666     8666              
  Partials     1150     1150              

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

[Enkidu93]

@Enkidu93 made 1 comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on ddaspit and pmachapman).

---

src/SIL.Machine/Corpora/ScrVersExtensions.cs line 90 at r1 (raw file):

This method seems pretty intense as it loads every verse reference as a struct into memory instead of just the differences?

It is definitely a little intense. Are you suggesting a way to just load the differences? I'm not sure what that would look like. I don't have access to the mappings etc. directly.

After looking at the Versification source code, another approach you could consider ScrVers.Save() which calls Versification.WriteToStream() to generate a string you could compare:...

I did consider this alternative. It looked to me like there were semantically identical versifications that could generate different strings, but I will confirm this.

And set when I set a breakpoint on return false; thisVerse was ESG 4:20, instead of Matthew 1:5 or Matthew 1:6 which I expected it to return false on?

Hmm, if I decide to stick with this approach, I will try to recreate. I do wonder if we even want to include differences of segment definition as real differences? 🤔

[Enkidu93]

@Enkidu93 made 1 comment.
Reviewable status: 1 of 3 files reviewed, 1 unresolved discussion (waiting on ddaspit and pmachapman).

---

src/SIL.Machine/Corpora/ScrVersExtensions.cs line 90 at r1 (raw file):

Previously, Enkidu93 (Eli C. Lowry) wrote…

This method seems pretty intense as it loads every verse reference as a struct into memory instead of just the differences?

It is definitely a little intense. Are you suggesting a way to just load the differences? I'm not sure what that would look like. I don't have access to the mappings etc. directly.

After looking at the Versification source code, another approach you could consider ScrVers.Save() which calls Versification.WriteToStream() to generate a string you could compare:...

I did consider this alternative. It looked to me like there were semantically identical versifications that could generate different strings, but I will confirm this.

And set when I set a breakpoint on return false; thisVerse was ESG 4:20, instead of Matthew 1:5 or Matthew 1:6 which I expected it to return false on?

Hmm, if I decide to stick with this approach, I will try to recreate. I do wonder if we even want to include differences of segment definition as real differences? 🤔

OK, to follow up.

The Save() method, unfortunately, does not sort excluded verses so these two custom versifications would not be equivalent:

        ScrVers customVrs1;
        using (CorporaUtils.VersificationLock.Lock())
        {
            string src = "MAT 1:2 = MAT 1:1\nMAT 1:1 = MAT 1:2\n-EXO 25:6\n-EXO 28:23";
            using var reader = new StringReader(src);
            customVrs1 = Versification.Table.Implementation.Load(reader, "vers.txt", ScrVers.English, "custom1");
            Versification.Table.Implementation.RemoveAllUnknownVersifications();
        }

        ScrVers customVrs2;
        using (CorporaUtils.VersificationLock.Lock())
        {
            string src = "MAT 1:2 = MAT 1:1\nMAT 1:1 = MAT 1:2\n-EXO 28:23\n-EXO 25:6";
            using var reader = new StringReader(src);
            customVrs2 = Versification.Table.Implementation.Load(reader, "vers.txt", ScrVers.English, "custom1");
            Versification.Table.Implementation.RemoveAllUnknownVersifications();
        }

Maybe we're ok with that since they are likely to be in order in the custom.vrs? I don't know, but this was my reservation about using the string representation

Also, regarding the ESG problem, this is just a goofy problem with English versification in general. new VerseRef("ESG 4:20", emptyEnglishVrs).ChangeVersification(ScrVers.English) also gives ESG 4:21. I've fixed this by converting both references to the same vrs.

Finally, what do you think about segment differences counting? Maybe we should just count those as real differences? It's one thing if they are in the mappings, but it doesn't seem like a big difference if they are just defined for a verse 🤷.

[pmachapman]

@pmachapman reviewed 2 files and all commit messages, and made 2 comments.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on ddaspit and Enkidu93).

---

src/SIL.Machine/Corpora/ScrVersExtensions.cs line 90 at r1 (raw file):

The Save() method, unfortunately, does not sort excluded verses

Thank you for looking into this - I hope I didn't lead you down a rabbit trail.

Finally, what do you think about segment differences counting?

It probably won't affect a draft too much, so is probably not worth flagging given the extra processing to check this (would you need to when checking each verse check the value of HasSegmentsDefined too, then check the segments?).

I don't think we need it for our use case, but you could add a parameter to check for segments if you feel motived?

[ddaspit]

@ddaspit reviewed 3 files and all commit messages, and made 2 comments.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on Enkidu93).

---

src/SIL.Machine/Corpora/ScrVersExtensions.cs line 74 at r3 (raw file):

                (VerseRef thisVerse, VerseRef otherVerse) in scrVers
                    .AllIncludedVerses()
                    .Zip(other.AllIncludedVerses())

The Zip will stop iterating on the shortest verse list, so if the versifications have different number of verses, it can still be counted as equivalent if the prefix is the same.

---

src/SIL.Machine/Corpora/ScrVersExtensions.cs line 75 at r3 (raw file):

                    .AllIncludedVerses()
                    .Zip(other.AllIncludedVerses())
                    .Select(tup => (tup.Item1, tup.Item2))

Nit: I don't think the Select operation is necessary.

[Enkidu93]

@Enkidu93 made 3 comments.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on ddaspit and pmachapman).

---

src/SIL.Machine/Corpora/ScrVersExtensions.cs line 90 at r1 (raw file):

Previously, pmachapman (Peter Chapman) wrote…

The Save() method, unfortunately, does not sort excluded verses

Thank you for looking into this - I hope I didn't lead you down a rabbit trail.

Finally, what do you think about segment differences counting?

It probably won't affect a draft too much, so is probably not worth flagging given the extra processing to check this (would you need to when checking each verse check the value of HasSegmentsDefined too, then check the segments?).

I don't think we need it for our use case, but you could add a parameter to check for segments if you feel motived?

No, no, rabbit trails are good if they ensure thoroughness.

OK, I'll leave it and if we decide that this is an important difference later, we can add it.

---

src/SIL.Machine/Corpora/ScrVersExtensions.cs line 74 at r3 (raw file):

Previously, ddaspit (Damien Daspit) wrote…

The Zip will stop iterating on the shortest verse list, so if the versifications have different number of verses, it can still be counted as equivalent if the prefix is the same.

Done. I used Concat() but if you'd prefer, I could just compare Count()s beforehand since, although it will cause it to be enumerated again, it's likely that different versifications will have a different number of verses.

---

src/SIL.Machine/Corpora/ScrVersExtensions.cs line 75 at r3 (raw file):

Previously, ddaspit (Damien Daspit) wrote…

Nit: I don't think the Select operation is necessary.

It's yelling at me that it can't deconstruct Tuple<VerseRef, VerseRef> . Maybe I'm missing something?

[ddaspit]

@ddaspit reviewed 1 file and all commit messages, made 1 comment, and resolved 1 discussion.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on Enkidu93 and pmachapman).

---

src/SIL.Machine/Corpora/ScrVersExtensions.cs line 75 at r3 (raw file):

Previously, Enkidu93 (Eli C. Lowry) wrote…

It's yelling at me that it can't deconstruct Tuple<VerseRef, VerseRef> . Maybe I'm missing something?

Try adding using System;.

[Enkidu93]

@Enkidu93 made 1 comment and resolved 1 discussion.
Reviewable status: 2 of 3 files reviewed, 1 unresolved discussion (waiting on ddaspit and pmachapman).

---

src/SIL.Machine/Corpora/ScrVersExtensions.cs line 75 at r3 (raw file):

Previously, ddaspit (Damien Daspit) wrote…

Try adding using System;.

Done 🤦‍♂️