Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SAM: add a sentence on case-insensitivity of RG PL (PR #684) #684

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jkbonfield
Copy link
Contributor

This is not changing what is valid / permitted, and indeed this hopefully clarifies it further. However the practicality of dealing with wide-spread non-compliant data with lowercase PL values is that tools may wish to be lenient and use case-insensitive matching.

Fixes #679

@github-actions
Copy link

github-actions bot commented Nov 9, 2022

Changed PDFs as of 86da59b: SAMv1 (diff).

zaeleus added a commit to zaeleus/noodles that referenced this pull request Nov 17, 2022
…itive

This is pre-emptively testing a spec change that allows this. See
samtools/hts-specs#684.
@jmarshall
Copy link
Member

I thought in the meeting we coalesced on the "also accept lowercase" slightly more conservative wording, rather than "accept case insensitively" — though to be sure the effect is similar.

@jkbonfield
Copy link
Contributor Author

That's possible. I see we now have Otter-AI meeting transcripts as minutes, but I haven't got access (sent a request off).

I'm happy to reword it. Thanks for the comments.

jkbonfield added a commit to jkbonfield/hts-specs that referenced this pull request Jan 3, 2023
This is not changing what is valid / permitted, and indeed this
hopefully clarifies it further.  However the practicality of dealing
with wide-spread non-compliant data with lowercase PL values is that
tools may wish to be lenient and use case-insensitive matching.

Fixes samtools#679
@jkbonfield
Copy link
Contributor Author

Updated wording.

jkbonfield added a commit to jkbonfield/hts-specs that referenced this pull request Jan 3, 2023
This is not changing what is valid / permitted, and indeed this
hopefully clarifies it further.  However the practicality of dealing
with wide-spread non-compliant data with lowercase PL values is that
tools may wish to be lenient and use case-insensitive matching.

Fixes samtools#679
zaeleus added a commit to zaeleus/noodles that referenced this pull request Jan 12, 2023
@jkbonfield jkbonfield added the sam label Feb 2, 2023
@jmarshall
Copy link
Member

Also need to update test/sam/failed/hdr.RG6.sam, which lists this file as failed due to PL:illumina.

This is not changing what is valid / permitted, and indeed this
hopefully clarifies it further.  However the practicality of dealing
with wide-spread non-compliant data with lowercase PL values is that
tools may wish to be lenient and use case-insensitive matching.

Also removes test/sam/failed/hdr.RG6.sam due to explicitly testing
against the use of lower-case PL fields.  While strictly not
conforming, it's overly harsh if we are advocating a more
spec-tolerant testing regime for PL.

Fixes samtools#679
@jkbonfield
Copy link
Contributor Author

This raises an interesting question on the use of the test data. From above:

This is not changing what is valid / permitted, ...

I'm hesitant to move test/sam/failed/hdr.RG6.sam to the test/sam/passed directory, as that implies we consider it to be valid. We are clearly still stating that it is not strictly valid. However we are saying that tools need to be lenient and should consider being pragmatic and implement checks in a case-insensitive manner due to widespread non-adherance to the specification.

I feel if we wish to have lowercase in the passed folder, then we're pretty much also stating that we should just make the rule case insensitive and drop the statement that it should be in uppercase.

So I've simply removed the test file, as a middle ground.

Ideally we'd have passed, failed, and warn, but personally I don't have the bandwidth to do that justice. I think it's more in the grey area that Daniel is exploring with StrictSAM, and there are many severity layers of warning too (some of which in my personal view come under the "just write your tool to be more robust" category).

@github-actions
Copy link

github-actions bot commented May 2, 2023

Changed PDFs as of f379895: SAMv1 (diff).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Needs review
Development

Successfully merging this pull request may close these issues.

sam: Clarify case-sensitivity of read group platform values
2 participants