IHTSDO / snow-owl

Snow Owl Terminology Server. This version is no longer maintained. Please use the upstream version or the alternate Snowstorm Terminology Server.
Apache License 2.0
11 stars 1 forks source link

Fix inferred relationship group numbering for new groups #30

Closed apeteri closed 7 years ago

apeteri commented 7 years ago

To prevent frequent changes of group numbers on inferred relationships, classification results are compared against the currently persisted set. If the same group of relationships can be found in both sets, the original group number is preserved.

Andrew reported an issue related to classification, which was found when comparing inferred relationships in the terminology server against the contents of SRS:

To fix the issue, new relationships which have a zero group number should be excluded from preservation attempts.

kaicode commented 7 years ago

@apeteri Thanks for the quick turnaround on this solution.

Could I ask for couple more assertions please? In the existing unit test it would be good to assert that the new group of the associated morphology is indeed group 0. I'm also wondering if there another scenario where a relationship may move to non-zero group.

apeteri commented 7 years ago

Consider one of the problematic concepts (on the left, the expected structure, on the right, the currently existing inferred relationships):

Neonatal osteopenia

The left-hand representation is always computed for each concept; the information comes from the reasoner (inferred direct parents) and the stated view (non-redundant "groups" of stated non-IS A relationships, reachable from this concept and all of its inferred indirect parents, up to the root concept, including single relationships with a group number of 0).

These results are then compared against the existing inferred view, to avoid rotation of group numbers (the generated left-hand side is always numbered sequentially, starting from group 1, whereas the existing right-hand side might have a single group with the identifier 4).

Groups are considered to be equal if they contain the same number of relationships, with each relationship having an equal relationship type and destination SCTID.

If a group from the left-hand side is found to be equal with a group on the right, the group number is carried over to the left. If this number in turn is already in use by another group on the left, the two groups swap their numbers to avoid collision.

The issue only occurs when an existing single inferred relationship retains its non-zero group number, even though no other relationship remains in the same (non-zero) group; this can happen either through automatic merge changes, manual updates or plain re-modeling through stated relationship changes.

In this example, "Occurrence = Neonatal" remains in group 1, and we have the same relationship on the "proposed inferred" left-hand side with group 0. Both groups have a single relationship with the same type and destination, so without the additional check in the PR, the "proposed inferred" output sets the group number to 1, and the group holding number 1 previously is set to group 0; this results in a steady, but incorrect state of inferred relationships on the concept. As Andrew wrote, if the inferred relationships are removed first, there is nothing to compare the "proposed inferred" output to, and everything goes back to normal.

I can't think of a manifestation of the "opposite" scenario, as in that case you'd have a different number of relationships in a group on each side, which would not be considered equal.

kaicode commented 7 years ago

I fully understand the manifestation of the issue after reading this. I'll add a couple more assertions after accepting the pull request for piece of mind and to solidify my understanding. Thanks @apeteri !