Extend Mm tag to allow callers to indicate unknown positions #592

jts · 2021-08-25T14:30:00Z

This draft PR addresses the issue with the Mm tag raised in #582. It adds an optional . or ? to change how downstream tools should interpret the skipped bases. When ? is present, nothing should be assumed about the modification status of the skipped bases. This proposal is slightly different than what we originally discussed in #582:

the flag is now optional as suggested by @jkbonfield
the flag now comes after the modification code, as I found this to be nicer when optional
I added an example of how ? should be interpreted
I added a footnote to guide interpretation of the (lack of) modification probabilities for skipped bases in . mode

Also, I reworded some existing text to reflect the uncertainty present in either mode. In some places the existing text implied that any bases not skipped are modified, but this needs to be determined by looking at the Ml tag to see if the probability is > 0.5. So I added words like "potentially modified" in a number of places.

hts-specs-bot · 2021-08-25T14:34:11Z

Changed PDFs as of 59b785e: SAMtags (diff).

jts · 2021-09-27T13:44:04Z

Hi @jkbonfield, could I gently bump this? I'd like to release a new version of nanopolish with modbam support, and would like to see this merged in (if acceptable) before I make that release.

jkbonfield · 2021-09-29T13:38:01Z

My apologies Jared. I've been concentrating my efforts elsewhere and this has slipped. I'll take a look.

jkbonfield

Agreed with the principle, but there a number of errors in the descriptions to fix.

jkbonfield · 2021-09-29T13:45:21Z

SAMtags.tex

+Following the base modification codes is an optional {\tt .} or {\tt ?} describing how skipped seq bases of the stated base type should be interpreted by downstream tools. 
+When this flag is {\tt ?} there is no information about the modification status of the skipped bases provided.
+When this flag is not present, or it is {\tt .}, these bases should be assumed to have low probability of modification\footnote{The decision whether a base is assumed to be unmodified or has a probability explicitly provided is up to the modification calling program. Some programs will elide calls with modification probabilites below a threshold to provide a more compact modification tag.}.
+This is then followed by a comma separated list of how many seq bases of the stated base type to skip, stored as a delta to the last and starting with 0 as the first (or next) base, starting from the uncomplemented 5' end of the {\sf SEQ} field.


I like the wording of the [.?]? optional code. Thanks.

Also good catch on the "unmodified seq bases" to "seq bases" change. If we have multiple modification strings, we're counting the ones not listed as potentially being modified by this code, note all codes, plus obviously we don't actually know with certainty which really are modified.

jkbonfield · 2021-09-29T13:49:40Z

SAMtags.tex

 \hfill\\
 The first character is the unmodified ``fundamental'' base as reported
 by the sequencing instrument for the top strand.
 It must be one of {\tt A}, {\tt C}, {\tt G}, {\tt T}, {\tt U} (if RNA) or {\tt N} for anything else, including any IUPAC ambiguity codes in the reported SEQ field.
 Note {\tt N} may be used to match any base rather than specifically an {\tt N} call by the sequencing instrument.
 This may be used in situations where the base modification is not a derivation of a standard base type.
-This is followed by either plus or minus indicating the strand the modification was observed on (relative to the original sequenced strand of {\sf SEQ} with plus meaning same orientation),\footnote{Hence a tool that may reverse complement sequences does not need to understand how to manipulate the {\tt Mm} and {\tt Ml} tags.} and one or more base modification codes.
-This is then followed by a comma separated list of how many unmodified seq bases of the stated base type to skip, stored as a delta to the last and starting with 0 as the first (or next) base, starting from the uncomplemented 5' end of the {\sf SEQ} field.
+This is followed by either plus or minus indicating the strand the modification was analysed for (relative to the original sequenced strand of {\sf SEQ} with plus meaning same orientation),\footnote{Hence a tool that may reverse complement sequences does not need to understand how to manipulate the {\tt Mm} and {\tt Ml} tags.} and one or more base modification codes.


I'm not sure I like the "observed on" to "analysed for" change. My instinct is we're recording data observations, and how that observation was derived is neither here nor there. This isn't a strong feeling though, so if you are particularly focused on wanting "analysed for" then I could be persuaded.

I'll revert this back to "observed on". I changed this as "the modification was observed on" felt too definitive to me but I think it is clear enough from that context that it is OK

jkbonfield · 2021-09-29T13:58:49Z

SAMtags.tex

+The first 5 {\tt C} bases are unmodified and the 6th, 19th and 20th have modification status indicated by the corresponding probabilities in the {\tt Ml} tag. The 12 cytosines between the 6th and 19th cytosine are unmodified. Modification probabilities for the 17 skipped cytosines are not provided.
+
+When the {\tt ?} flag is present the tag {\tt C+.m?,5,12,0;} tells us the modification status of the first five 
+cytosine bases is unknown, the sixth cytosine is called, followed by 12 more unknown cytosines, and the 19th and 20th are called.


One slight wording change here maybe is that with C+m? when we say "the sixth cytosine is called" we mean "the status of sixth cytosince has been recorded" (which may or may not be called as a likely base modification). I get what you mean by "called", but it may be misinterpreted by others as evidence of a positive modification event. Or perhaps keep the word called but with an addition: eg "is called (as either modified or unmodified)"?

Good catch, I'll take your suggestion to add "(as either modified or unmodified)"

jkbonfield · 2021-09-29T14:03:18Z

SAMtags.tex

+potential 5-Methylcytosine bases on the top strand of {\sf SEQ}.
+The first 5 {\tt C} bases are unmodified and the 6th, 19th and 20th have modification status indicated by the corresponding probabilities in the {\tt Ml} tag. The 12 cytosines between the 6th and 19th cytosine are unmodified. Modification probabilities for the 17 skipped cytosines are not provided.
+
+When the {\tt ?} flag is present the tag {\tt C+.m?,5,12,0;} tells us the modification status of the first five 


C+.m?,5,12,0; has both . and ? present, in two different locations. I don't understand this syntax. Did you mean C+?m,5,12,0;?

Sorry, the . were leftover from my first pass, which had a mandatory flag. I'll fix these to be C+m?.

jkbonfield · 2021-09-29T14:08:58Z

SAMtags.tex

 Given the unmodified base already has a phred likelihood, this base modification quality should be interpreted as the likelihood of this modification being correct given an assumption the original call is correct.

 \begin{description}
-\item[Mm:Z:\tagregex{([ACGTUN][-+]([a-z]+|[0-9]+)(,[0-9]+)*;)*}]
+\item[Mm:Z:\tagregex{([ACGTUN][-+]([a-z]+|[0-9]+)(,[0-9]+)*[.?]?;)*}]


This regexp is wrong.

Breaking it down, the original was canonical_base [ACGTUN], strand [+-], a list of modified base types or CHEBI ID ([a-z]+|[0-9]+), followed by a comma-separated series of deltas: (,[0-9]+)* ending in a semicolon.

As far as I understand the intention, the . or ? appear between the strand and base type list, so we'd want:

([ACGTUN][-+]([a-z]+|[0-9]+)[.?]?(,[0-9]+)*;)*

Thanks for catching this. My intention is for the flag to appear after the base modification code - which I think is what your proposed regex has? My regex is quite rusty...

jkbonfield · 2021-09-29T14:12:44Z

SAMtags.tex

@@ -530,10 +537,10 @@ \subsection{Base modifications}
 When multiple modifications are listed, for example {\tt C+mh,5,12,0;}, it indicates the modification may be any of the stated bases.
 The associated confidence values in the {\tt Ml} tag may be used to determine the relative likelihoods between the options.
 The example above is equivalent to {\tt C+m,5,12,0;C+h,5,12,0;}, although this will have a different ordering of confidence values in {\tt Ml}.
-Note ChEBI codes cannot be used in the multi-modification form (such as the {\tt C+mh} example above).
+Note ChEBI codes cannot be used in the multi-modification form (such as the {\tt C+.mh} example above).


Is there a C+.mh example above? We have (if fixed) a C+?mh one though.

I'll fix this to C+mh, the . shouldn't have been there.

jts · 2021-09-29T14:23:16Z

Thanks James! I'm teaching a workshop today but will address these soon.

jkbonfield · 2021-09-29T14:48:29Z

Thanks James! I'm teaching a workshop today but will address these soon.

No problem. I'm in no position to comment on slow responses. :)

hts-specs-bot · 2021-09-30T16:51:02Z

Changed PDFs as of 6854fe0: SAMtags (diff).

jkbonfield · 2021-10-07T14:05:14Z

Thanks Jared. I'm happy with these amendments now.

As the SAM spec co-maintainer, I'll ping @jmarshall incases he wishes to comment too. However consider the general gist of it as accepted as we've previously discussed it in our meetings and the general consensus is it's the right move. If we change anything it's likely to be minor wording.

jts · 2021-10-12T13:28:43Z

Great, thanks James.

jmarshall

I'm happy with the proposed notation, and pleased that the existing syntax without any .? flag character remains accepted.

I've pointed out a couple of formatting issues. There are a few more, but as some of those are already present in the existing text I'm happy to merge this as is (ideally with the suggested formatting fixes) and address those other concerns myself separately.

jmarshall · 2021-12-08T22:26:04Z

SAMtags.tex

-This is then followed by a comma separated list of how many unmodified seq bases of the stated base type to skip, stored as a delta to the last and starting with 0 as the first (or next) base, starting from the uncomplemented 5' end of the {\sf SEQ} field.
+Following the base modification codes is an optional {\tt .} or {\tt ?} describing how skipped seq bases of the stated base type should be interpreted by downstream tools. 
+When this flag is {\tt ?} there is no information about the modification status of the skipped bases provided.
+When this flag is not present, or it is {\tt .}, these bases should be assumed to have low probability of modification\footnote{The decision whether a base is assumed to be unmodified or has a probability explicitly provided is up to the modification calling program. Some programs will elide calls with modification probabilites below a threshold to provide a more compact modification tag.}.


The convention in this document is for footnotes to go after the punctuation.

jmarshall · 2021-12-08T22:30:16Z

SAMtags.tex

 \hfill\\
 The first character is the unmodified ``fundamental'' base as reported
 by the sequencing instrument for the top strand.
 It must be one of {\tt A}, {\tt C}, {\tt G}, {\tt T}, {\tt U} (if RNA) or {\tt N} for anything else, including any IUPAC ambiguity codes in the reported SEQ field.
 Note {\tt N} may be used to match any base rather than specifically an {\tt N} call by the sequencing instrument.
 This may be used in situations where the base modification is not a derivation of a standard base type.
 This is followed by either plus or minus indicating the strand the modification was observed on (relative to the original sequenced strand of {\sf SEQ} with plus meaning same orientation),\footnote{Hence a tool that may reverse complement sequences does not need to understand how to manipulate the {\tt Mm} and {\tt Ml} tags.} and one or more base modification codes.
-This is then followed by a comma separated list of how many unmodified seq bases of the stated base type to skip, stored as a delta to the last and starting with 0 as the first (or next) base, starting from the uncomplemented 5' end of the {\sf SEQ} field.
+Following the base modification codes is an optional {\tt .} or {\tt ?} describing how skipped seq bases of the stated base type should be interpreted by downstream tools. 


Quote marks around the literal characters, as so (and similarly in nearby lines):

Suggested change

Following the base modification codes is an optional {\tt .} or {\tt ?} describing how skipped seq bases of the stated base type should be interpreted by downstream tools.

Following the base modification codes is an optional `{\tt .}' or `{\tt ?}' describing how skipped seq bases of the stated base type should be interpreted by downstream tools.

jkbonfield · 2021-12-09T09:05:17Z

Thanks for the review John.

I'm happy with the suggested formatting changes, so if you wish to edit and merge yourself please do so. Or if you want me to make the changes let me know and I'll do so (and merge).

jkbonfield · 2021-12-09T09:08:58Z

We did also say we were going to make them official, so mM to MM etc. We gave warning on this (#418 (comment) - long overdue), and this (welcomed) tweak was one reason for delay. I think once this is merged we should also make the switch to official name space too, in its own commit.

Fixes #582

jkbonfield · 2021-12-09T15:50:32Z

Rebased, squashed and merged as 71241fe, with John's footnote fix and quoting tidyups (and a few more nearby).

jmarshall · 2021-12-13T11:49:44Z

Pushed 6835cd3 with a title page recording a commit that exists on master. PRs don't need to include updated PDFs (this will be done separately when they are merged), and draft PDFs containing commit hashes that exist only pre-rebasing should be undone during merging… 😄

jts added 3 commits August 18, 2021 10:50

first pass at addressing samtools#582

ea2784b

Fix missing flag in example

0e24430

updates following the discussion in samtools#582

59b785e

jts added a commit to jts/mbtools that referenced this pull request Aug 25, 2021

Support the Mm flag proposed in samtools/hts-specs#592

6f4b4d7

jmarshall added the sam-tags label Sep 1, 2021

jts mentioned this pull request Sep 22, 2021

Support for bam output of methylation via the new MM tag? jts/nanopolish#939

Closed

jkbonfield requested changes Sep 29, 2021

View reviewed changes

updates after @jkbonfield's comments

6854fe0

jkbonfield approved these changes Oct 7, 2021

View reviewed changes

jkbonfield added the sam label Oct 15, 2021

jmarshall approved these changes Dec 8, 2021

View reviewed changes

jkbonfield pushed a commit that referenced this pull request Dec 9, 2021

Extend Mm tag to allow callers to indicate unknown positions (PR #592)

71241fe

Fixes #582

jkbonfield closed this Dec 9, 2021

jmarshall added the Merged For marking PRs that have been merged outwith GitHub and are shown as merely “Closed” label Dec 13, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extend Mm tag to allow callers to indicate unknown positions #592

Extend Mm tag to allow callers to indicate unknown positions #592

jts commented Aug 25, 2021

hts-specs-bot commented Aug 25, 2021

jts commented Sep 27, 2021

jkbonfield commented Sep 29, 2021

jkbonfield left a comment

jkbonfield Sep 29, 2021

jkbonfield Sep 29, 2021

jts Sep 30, 2021

jkbonfield Sep 29, 2021

jts Sep 30, 2021

jkbonfield Sep 29, 2021

jts Sep 30, 2021

jkbonfield Sep 29, 2021

jts Sep 30, 2021

jkbonfield Sep 29, 2021

jts Sep 30, 2021

jts commented Sep 29, 2021

jkbonfield commented Sep 29, 2021

hts-specs-bot commented Sep 30, 2021

jkbonfield commented Oct 7, 2021

jts commented Oct 12, 2021

jmarshall left a comment

jmarshall Dec 8, 2021

jmarshall Dec 8, 2021

jkbonfield commented Dec 9, 2021

jkbonfield commented Dec 9, 2021

jkbonfield commented Dec 9, 2021

jmarshall commented Dec 13, 2021

	Following the base modification codes is an optional {\tt .} or {\tt ?} describing how skipped seq bases of the stated base type should be interpreted by downstream tools.
	Following the base modification codes is an optional `{\tt .}' or `{\tt ?}' describing how skipped seq bases of the stated base type should be interpreted by downstream tools.

Extend Mm tag to allow callers to indicate unknown positions #592

Extend Mm tag to allow callers to indicate unknown positions #592

Conversation

jts commented Aug 25, 2021

hts-specs-bot commented Aug 25, 2021

jts commented Sep 27, 2021

jkbonfield commented Sep 29, 2021

jkbonfield 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

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

Choose a reason for hiding this comment

jts commented Sep 29, 2021

jkbonfield commented Sep 29, 2021

hts-specs-bot commented Sep 30, 2021

jkbonfield commented Oct 7, 2021

jts commented Oct 12, 2021

jmarshall left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkbonfield commented Dec 9, 2021

jkbonfield commented Dec 9, 2021

jkbonfield commented Dec 9, 2021

jmarshall commented Dec 13, 2021