Removing line that breaks Joel.4.1-Joel.4.21=Joel.3.1-Joel.3.21. These a...

[chrisburrell]

...re not identity mappings. Furthermore, identity mappings have to be mapped to preserve verse parts stored in the qualified keys, rather than the verse

There are two issues with these lines:

1- it breaks when two mappings map to the same verses in different chapters (see ref in commit message) - observed as breaking Joel 4 in STEP

2- the qualified key may be storing extra information that would get lost. (assumed for code reading).

As a result of 2-, it is best to remove the line. Chris

[cloudbees-pull-request-builder]

jsword #66 SUCCESS This pull request looks good

[chrisburrell]

Actually - scanning the code, it looks like the sub-part has been moved to the verse... So perhaps it's not a big deal, however, I question how much saving we make here.

[dmsmith]

Regarding adding subpart to Verse, this was to improve JSword to match the OSIS specification.

There are other parts to an OSIS Verse specification: work and grain. Not inclined to add these unless there is a real use case.

[chrisburrell]

Understand that. Although the concept of subparts in the versification mappings is slightly different to the one in verses commonly used...

e.g.

we would we want to be able to distinguish part !a in versification A to part !a in versification b if they don't quite line up. The whole idea was to make the subparts as flexible as possible,

So you could have

Versification A=>KJV a1=k1!a a2=k1!b

Versification B=>KJV b1=>k1!c b2=>k1!d

This would ensure that mapping from A=>B, we would not end up with a1 =>b1 but instead would end up with a1=>k1=>b1-b2

So we can still do that, but it's slightly confusing to have !c and !d... We used to be able to have @esvFirstSentence (i guess we still can).

Cheers Chris

On 29 March 2014 14:34, dmsmith notifications@github.com wrote:

Regarding adding subpart to Verse, this was to improve JSword to match the OSIS specification.

There are other parts to an OSIS Verse specification: work and grain. Not inclined to add these unless there is a real use case.

Reply to this email directly or view it on GitHubhttps://github.com/crosswire/jsword/pull/72#issuecomment-38997188 .

[chrisburrell]

If anything, on performance on small devices, we would want to fix the mapping files to avoid doing the processing at all on start-up, so perhaps this warrants a small unit test to test that mappings are as small as possible.

[dmsmith]

All good points.

Regarding mapping. There are only a couple of externally facing routines regarding mapping. That is, routines that should be used by a front-end. These mapping functions essentially take a verse from one v11n and give the equivalent in another using KJV (the right hand side in the mapping files) either as a source, destination or intermediate. These functions regard verses as a whole and return verses as a whole. If given a sub part, it has to be ignored. Internally, it is as you say, the sub-part in the map has to be used to get the right mapping.

Regarding performance, serialization would probably give better performance than a file that has to be compiled into an internal representation. The file gives better support. I've often thought about having both and comparing timestamps to determine whether the serialization needs to be updated. There are several places it would help.

On a different point, I'd like to get rid of 0 in the mapping files replacing them with two entries. The notion of 0 here is not orthogonal to what JSword/SWORD views it to be, that is an introduction.

[chrisburrell]

I agree with the removal of verse 0, it's confusing.

On 29 March 2014 16:05, dmsmith notifications@github.com wrote:

All good points.

Regarding mapping. There are only a couple of externally facing routines regarding mapping. That is, routines that should be used by a front-end. These mapping functions essentially take a verse from one v11n and give the equivalent in another using KJV (the right hand side in the mapping files) either as a source, destination or intermediate. These functions regard verses as a whole and return verses as a whole. If given a sub part, it has to be ignored. Internally, it is as you say, the sub-part in the map has to be used to get the right mapping.

Regarding performance, serialization would probably give better performance than a file that has to be compiled into an internal representation. The file gives better support. I've often thought about having both and comparing timestamps to determine whether the serialization needs to be updated. There are several places it would help.

On a different point, I'd like to get rid of 0 in the mapping files replacing them with two entries. The notion of 0 here is not orthogonal to what JSword/SWORD views it to be, that is an introduction.

Reply to this email directly or view it on GitHubhttps://github.com/crosswire/jsword/pull/72#issuecomment-38999848 .

[chrisburrell]

... and stems from my original misunderstanding of what verse 0 really meant.

On 29 March 2014 16:17, Chris Burrell chris@burrell.me.uk wrote:

I agree with the removal of verse 0, it's confusing.

On 29 March 2014 16:05, dmsmith notifications@github.com wrote:

All good points.

Regarding mapping. There are only a couple of externally facing routines regarding mapping. That is, routines that should be used by a front-end. These mapping functions essentially take a verse from one v11n and give the equivalent in another using KJV (the right hand side in the mapping files) either as a source, destination or intermediate. These functions regard verses as a whole and return verses as a whole. If given a sub part, it has to be ignored. Internally, it is as you say, the sub-part in the map has to be used to get the right mapping.

Regarding performance, serialization would probably give better performance than a file that has to be compiled into an internal representation. The file gives better support. I've often thought about having both and comparing timestamps to determine whether the serialization needs to be updated. There are several places it would help.

On a different point, I'd like to get rid of 0 in the mapping files replacing them with two entries. The notion of 0 here is not orthogonal to what JSword/SWORD views it to be, that is an introduction.

Reply to this email directly or view it on GitHubhttps://github.com/crosswire/jsword/pull/72#issuecomment-38999848 .

[mjdenham]

Please do not totally remove verse 0 mapping. A current verse of 0 is a valid state, at least in And Bible, and mapping from verse 0 in a v11n to another v11n needs to return the appropriate verse.

As part of And Bible's build process it comments out zerosUnmapped from all v11n mapping files. That is the only way verse mapping will work.

We did discuss a change to the v11n mapping code for verse 0 on 29-30 Jan in JSword-devel/'Bookset' but I did not understand the suggested change and I don't think it was actioned.

[chrisburrell]

Hi Martin

I believe what we talked about ages ago is now fixed, as of 2 weeks ago. DM did some work on refactoring this, and I fixed a couple of bugs to do with last week and this week. Hoping the issues with AndBible should now be fixed. One to confirm I guess.... Verse 0 should now map to the most appropriate verse.

DM is referring to the notation of 'verse 0' in the mapping files. In the mapping files, 'verse 0' really means 'pre-verse 1'. My bad for introducing that.

Chris

On 29 March 2014 16:30, Martin notifications@github.com wrote:

Please do not totally remove verse 0 mapping. A current verse of 0 is a valid state, at least in And Bible, and mapping from verse 0 in a v11n to another v11n needs to return the appropriate verse.

As part of And Bible's build process it comments out zerosUnmapped from all v11n mapping files. That is the only way verse mapping will work.

We did discuss a change to the v11n mapping code for verse 0 on 29-30 Jan in JSword-devel/'Bookset' but I did not understand the suggested change and I don't think it was actioned.

Reply to this email directly or view it on GitHubhttps://github.com/crosswire/jsword/pull/72#issuecomment-39000632 .

[mjdenham]

Ah, that's great. I see the commit now. It also looks as if !zerosUnmapped is not used any more. I have done a few tests (testMapVerseZero below) and the problem does seem to have been fixed and the mapper will work fine for And Bible. I also tried some more ambitious tests with ranges that include verse 0 and consequently cross chapter boundaries and had some errors (testMapRangeIncludingVerseZero below), but I don't know if the mapper is supposed to cope with that situation:

/** These tests all work fine */
@Test
public void testMapVerseZero() throws NoSuchVerseException {
    doTest(KJV, "Ps.50.0", KJV, "Ps.50.0");
    doTest(KJV, "Ps.50.0", CATHOLIC, "Ps.50.0");
    doTest(KJV, "Ps.50.0", KJV, "Ps.50.0");
    doTest(KJV, "Ps.50.0", SYNODAL, "Ps.49.1");
    doTest(KJV, "Ps.50.1", SYNODAL, "Ps.49.1");
    doTest(SYNODAL, "Ps.49.1", KJV, "Ps.50.0-Ps.50.1");
}

/** These tests fail but I don't know if they should pass or if this functionality is required by you - AB does not require this sort of thing */
@Test
public void testMapRangeIncludingVerseZero() throws NoSuchVerseException {
    // Error: Too many parts to the Verse. (Parts are separated by any of Ps.9.21-Ps.9.22, Ps, 9, 21 Ps, 9, 22)
    doTest(SYNODAL, "Ps.9.21-Ps.9.22", KJV, "Ps.9.20-Ps.10.1");
    // Error: Too many parts to the verse
    doTest(KJV, "Ps.9.20-Ps.10.1", SYNODAL, "Ps.9.21-Ps.9.22");
    // Psalms 49:2-20 has a cardinality of 19 whilst Psalms 50:2-23 has a cardinality of 22.
    doTest(KJV, "Ps.49.20-Ps.50.1", SYNODAL, "Ps.48.20-Ps.49.1");
}