Last hour fixes #91

sveinugu · 2025-02-19T00:14:58Z

I should of course have done this earlier, but I have now read the spec detailedly and found a number of issues, errors, and inconsistencies.

The two most important of which is that:

the recommended bas schema includes the accession attribute. While I am not against this attribute per se, the issue on this is still open (New schema term: accessions #79) and there is nothing in the decision record about this. On the top of my head, I have at least two issues with this attribute (other than the formalities): 1. Should it allow for more than one accession per sequence? 2. Should the accessions somehow be namespaced, e.g. be in CURIE form?
The length attribute is now digested, even though it is not inherent. Related to this is the presentation of the digestion algorithm, where I think the filtering step should be moved to later in order to allow for the creation of level 1 digests for inherent attributes.

Please look at the different commits for other issues.

Hope there is still time to fix these issues!

…calculating the top-level digest

…hat create inconsistencies or are plainly incorrect now

…ell as consistent with RefSeq Sequences

…pairs

… to me this should depend on the use case/implementation

docs/seqcol.md

sveinugu · 2025-02-19T00:27:42Z

docs/seqcol.md

@@ -725,7 +725,7 @@ This digest allows the comparison to be pre-computed, and more easily compared.
 - inherent: false
 - collated: false
 - passthru: false
- transient: false
+- transient: true or false (depending on the use case/implementation)


The content is a duplicate of the sequences attribute, except for the sorting. IMO, the considerations to make this transient or not might include 1) storage space on server 2) performance/overhead on server of just-in-time sorting on requests, 3) whether client-side sorting is problematic for the use-case.

@nsheff Did you review this? If so, please resolve

nsheff · 2025-02-19T15:27:25Z

Decisions from today:

filtering should be moved to between steps 3 and 4
accessions should be removed for now (need to disambiguate sequence accessions from seqcol accessions)

nsheff

Just a few clarifications suggested on your edits, around the same_order section of the comparison results.

docs/compare_collections.md

nsheff · 2025-02-25T12:37:28Z

docs/compare_collections.md


 Because the sequence array would contain the same sequences, the comparison function will count them all as matching.
 Similarly, the names arrays contain the same names and so all will be counted as a match.
-However, the same_order will *not* be true, so it will yield false.
+However, the same_order will *not* be true; it will yield false for all attributes.


This is again incorrect. Here's an example:

https://refget.databio.org/comparison/fLf5M0BOIPIqcfbE6R8oYwxsy-PnoV32/E6zGtGuc8wKYmCMw5gaLW3ppyXsoO6p4

it does not yield false for all attributes. The lengths and sequences are in the same order.

so it will yield false only for the names attribute.

I agree

EDIT: I agree, based on your original example. My idea was to change the example slightly (which also unfortunately makes it more complex) so that the line "This is the same output as a comparison of two sequence collections in different orders" becomes true (see comment below)

nsheff · 2025-02-25T12:37:45Z

docs/compare_collections.md

@@ -56,15 +56,15 @@ Now that you know the basics, and once you have an understanding of what the com

 ## Limitation of the comparison function: distinguishing out-of-order from mismatched arrays

-One limitation of the comparison function is that it does comparisons at the level of arrays, not at the level of individual elements. What would the comparison function return for two sequence collections that are exact duplicates except for two component sequences that have swapped names?
+One limitation of the comparison function is that it does comparisons at the level of arrays, not at the level of individual elements. What would the comparison function return for two sequence collections that have the same content, but in different orders, AND where in addition two of the sequences have swapped names?


This correctly is not correct. This example was not about different orders, just about a name swap.

My suggestion is to change the example slightly, see: https://github.com/ga4gh/refget/pull/91/files/fc9624d21aef1486488fe89e5cfe1f3885848f29#r1969833123

nsheff · 2025-02-25T12:38:08Z

docs/compare_collections.md

@@ -56,15 +56,15 @@ Now that you know the basics, and once you have an understanding of what the com

 ## Limitation of the comparison function: distinguishing out-of-order from mismatched arrays

-One limitation of the comparison function is that it does comparisons at the level of arrays, not at the level of individual elements. What would the comparison function return for two sequence collections that are exact duplicates except for two component sequences that have swapped names?
+One limitation of the comparison function is that it does comparisons at the level of arrays, not at the level of individual elements. What would the comparison function return for two sequence collections that have the same content, but in different orders, AND where in addition two of the sequences have swapped names?


Suggested change

One limitation of the comparison function is that it does comparisons at the level of arrays, not at the level of individual elements. What would the comparison function return for two sequence collections that have the same content, but in different orders, AND where in addition two of the sequences have swapped names?

One limitation of the comparison function is that it does comparisons at the level of arrays, not at the level of individual elements. What would the comparison function return for two sequence collections that have the same content, but where two of the sequences have swapped names?

Another consequence of changing the example (see https://github.com/ga4gh/refget/pull/91/files/fc9624d21aef1486488fe89e5cfe1f3885848f29#r1969833123)

docs/compare_collections.md

nsheff · 2025-02-25T12:39:19Z

docs/compare_collections.md


 Because the sequence array would contain the same sequences, the comparison function will count them all as matching.
 Similarly, the names arrays contain the same names and so all will be counted as a match.
-However, the same_order will *not* be true, so it will yield false.
+However, the same_order will *not* be true; it will yield false for all attributes.


Suggested change

However, the same_order will *not* be true; it will yield false for all attributes.

However, the same_order will *not* necessarily be true; it will yield false for at least one attribute (the `names`).

This is another consequence of changing the example slightly

docs/seqcol.md

nsheff · 2025-02-25T12:52:40Z

@sveinugu I made a few suggestions on top of yours. Can you accept/confirm my corrections to your edits now? Then we can merge your changes.

sveinugu · 2025-02-25T13:55:43Z

docs/compare_collections.md


 Because the sequence array would contain the same sequences, the comparison function will count them all as matching.
 Similarly, the names arrays contain the same names and so all will be counted as a match.
-However, the same_order will *not* be true, so it will yield false.
+However, the same_order will *not* be true; it will yield false for all attributes.

 This is the same output as a comparison of two sequence collections in different orders, without the name swap. This is a fundamental limitation of the array-based method of comparing.


This is the same output as a comparison of two sequence collections in different orders, without the name swap.

My main problem is with this line. In the example where everything is the same except for a name swap, then the output will NOT be the same as a comparison of two sequence collections in different order. As you mention above, in the main example, only the same_order value for the names attribute will be false. In the comparison of two sequence collections in different orders, all same_order values will be false!

docs/compare_collections.md

sveinugu

Sorry for spamming you with single comments, I should have started the batch review straight away.

So I think we are on the same page except for the last example in the "Compare two sequence collections" (under "limitations"). I suggest to change the example to make the sentence "This is the same output as a comparison of two sequence collections in different orders" correct, which I do not think it is now, and all the small changes are just a consequence of that major change. However, there might be a simpler way to get the message across, as my suggestion makes the whole text slightly more complidated. Either way, perhaps adding an explicit JSON example will simplify things?

docs/compare_collections.md

sveinugu · 2025-02-25T14:12:22Z

docs/compare_collections.md

@@ -56,15 +56,15 @@ Now that you know the basics, and once you have an understanding of what the com

 ## Limitation of the comparison function: distinguishing out-of-order from mismatched arrays

-One limitation of the comparison function is that it does comparisons at the level of arrays, not at the level of individual elements. What would the comparison function return for two sequence collections that are exact duplicates except for two component sequences that have swapped names?
+One limitation of the comparison function is that it does comparisons at the level of arrays, not at the level of individual elements. What would the comparison function return for two sequence collections that have the same content, but in different orders, AND where in addition two of the sequences have swapped names?


My suggestion is to change the example slightly, see: https://github.com/ga4gh/refget/pull/91/files/fc9624d21aef1486488fe89e5cfe1f3885848f29#r1969833123

sveinugu · 2025-02-25T14:13:10Z

docs/compare_collections.md

@@ -56,15 +56,15 @@ Now that you know the basics, and once you have an understanding of what the com

 ## Limitation of the comparison function: distinguishing out-of-order from mismatched arrays

-One limitation of the comparison function is that it does comparisons at the level of arrays, not at the level of individual elements. What would the comparison function return for two sequence collections that are exact duplicates except for two component sequences that have swapped names?
+One limitation of the comparison function is that it does comparisons at the level of arrays, not at the level of individual elements. What would the comparison function return for two sequence collections that have the same content, but in different orders, AND where in addition two of the sequences have swapped names?


Another consequence of changing the example (see https://github.com/ga4gh/refget/pull/91/files/fc9624d21aef1486488fe89e5cfe1f3885848f29#r1969833123)

@nsheff

Changes from @nsheff I agree with Co-authored-by: Nathan Sheffield <[email protected]>

sveinugu · 2025-02-25T14:30:54Z

docs/compare_collections.md

@@ -66,5 +66,5 @@ This is the same output as a comparison of two sequence collections in different

 In this particular example, these results can be distinguished by the `sorted_name_length_pairs` attribute, because this would yield a perfect match for the second example, where all the pairs are intact but in a different order -- but it would NOT yield a match for the example with swapped names, because the name-length pairs would be different.

-This solves the issue for swapped names, but there is still potential for problems with other arrays or custom attributes. Therefore, we warn users that when the `_same_order` is flagged as false, this *does not imply that the pairs are intact*, and if this is a requirement, further investigation would be necessary. If distinguishing these scenarios is important, one possible solution would be to add another non-inherent array, similar to `sorted_name_length_pairs`, but including *all* attributes for each element rather than just the names and lengths. The comparison function would then immediately provide an answer as to whether the annotated sequence elements match *as units* between two collections.
+This solves the issue for swapped names, but there is still potential for problems with other arrays or custom attributes. Therefore, we warn users that when the `_same_order` is flagged as false, this *does not imply that the pairs are intact*, and if this is a requirement, further investigation would be necessary. If distinguishing these scenarios is important, one possible solution would be to add another non-inherent collated attribute, similar to `sorted_name_length_pairs`, but including *all* collated attributes for each element rather than just the names and lengths. The comparison function would then immediately provide an answer as to whether the annotated sequence elements match *as units* between two collections.


@nsheff The Github review process confused me. Here, I approved my own new suggestion, please review this independently.

sveinugu added 10 commits February 19, 2025 00:36

Remove accessions from the base schema

a1e0591

Move filtering step to between steps 3 and 4. Filter "length" before …

93f7e4f

…calculating the top-level digest

Misc statements that have not been cleaned up from earlier versions t…

de26248

…hat create inconsistencies or are plainly incorrect now

Updated example seqcols and digests to be internally consistent, as w…

ebdb2eb

…ell as consistent with RefSeq Sequences

Added a few links

095516e

Clean up the description of name_length_pairs and sorted_name_length_…

62356dd

…pairs

Is there a reason lengths is excluded here?

cc9d2bc

Should sorted_sequences be RECOMMENDED as transient: false? Seems…

d1abbdb

… to me this should depend on the use case/implementation

Smaller cleanup

54ed980

Logical errors in compare description

fc9624d

sveinugu requested a review from nsheff February 19, 2025 00:15

sveinugu commented Feb 19, 2025

View reviewed changes

docs/seqcol.md Show resolved Hide resolved

sveinugu commented Feb 19, 2025

View reviewed changes

nsheff approved these changes Feb 25, 2025

View reviewed changes

sveinugu commented Feb 25, 2025

View reviewed changes

docs/compare_collections.md Outdated Show resolved Hide resolved

sveinugu commented Feb 25, 2025

View reviewed changes

sveinugu and others added 2 commits February 25, 2025 15:25

Apply suggestions from code review

1d5763b

Changes from @nsheff I agree with Co-authored-by: Nathan Sheffield <[email protected]>

Updated suggestion

93dea51

sveinugu commented Feb 25, 2025

View reviewed changes

Revert suggestion on order-relaxed identity

bbf7a86

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Last hour fixes #91

Last hour fixes #91

sveinugu commented Feb 19, 2025

sveinugu Feb 19, 2025

sveinugu Feb 25, 2025

nsheff commented Feb 19, 2025 •

edited

Loading

nsheff left a comment

nsheff Feb 25, 2025

nsheff Feb 25, 2025

sveinugu Feb 25, 2025 •

edited

Loading

nsheff Feb 25, 2025

sveinugu Feb 25, 2025

nsheff Feb 25, 2025

sveinugu Feb 25, 2025

nsheff Feb 25, 2025

sveinugu Feb 25, 2025

nsheff commented Feb 25, 2025

sveinugu Feb 25, 2025

sveinugu left a comment

sveinugu Feb 25, 2025

sveinugu Feb 25, 2025

sveinugu Feb 25, 2025

	One limitation of the comparison function is that it does comparisons at the level of arrays, not at the level of individual elements. What would the comparison function return for two sequence collections that have the same content, but in different orders, AND where in addition two of the sequences have swapped names?
	One limitation of the comparison function is that it does comparisons at the level of arrays, not at the level of individual elements. What would the comparison function return for two sequence collections that have the same content, but where two of the sequences have swapped names?

	However, the same_order will not be true; it will yield false for all attributes.
	However, the same_order will not necessarily be true; it will yield false for at least one attribute (the `names`).

Last hour fixes #91

Are you sure you want to change the base?

Last hour fixes #91

Conversation

sveinugu commented Feb 19, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nsheff commented Feb 19, 2025 • edited Loading

nsheff left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sveinugu Feb 25, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nsheff commented Feb 25, 2025

Choose a reason for hiding this comment

sveinugu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nsheff commented Feb 19, 2025 •

edited

Loading

sveinugu Feb 25, 2025 •

edited

Loading