-
Notifications
You must be signed in to change notification settings - Fork 100
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
Make use of isotope label information in InChi values in transition lists and .msp files #3230
base: master
Are you sure you want to change the base?
Conversation
…s, e.g. the "1D3" in "InChI=1S/C8H8O/c1-7(9)8-5-3-2-4-6-8/h2-6H,1H3/i1D3" which tells us that three of the hydrogens in C8H8O are deuterium
…k/20241102_use_InChi_labels
@nickshulman this one's pretty nerdy, but it needs the blessing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, but here are a few suggestions.
{ | ||
Dictionary<string, int> result = null; | ||
var inchi = GetInChI(); | ||
if (!string.IsNullOrEmpty(inchi)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd recommend doing something like:
if (string.IsNullOrEmpty(inchi))
return result;
so that the main body of the function is less indented.
@@ -102,6 +102,16 @@ public static Dictionary<string, string> FormatAccessionNumbers(string keysTSV, | |||
return keys; | |||
} | |||
|
|||
public static MoleculeAccessionNumbers Create(Dictionary<string, string> accessions) | |||
{ | |||
if (accessions==null || accessions.Count == 0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The spacing around the "==" is incorrect.
} | ||
if (Adduct.DICT_ADDUCT_ISOTOPE_NICKNAMES.TryGetValue(isotope, out var skylineIsotope)) // e.g. "C13" => "C'" | ||
{ | ||
if (result == null) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd recommend replacing this "if" and the two subsequent two "else" clauses with:
result ??= new Dictionary<string, int>();
result.TryGetValue(skylineIsotope, out var existing);
result[skylineIsotope] = existing + count;
@@ -344,4 +344,7 @@ | |||
<data name="NistLibraryBase_GetMod_Unknown_modification__0__at_line__1_" xml:space="preserve"> | |||
<value>Unknown modification {0} at line {1}</value> | |||
</data> | |||
<data name="NistLibraryBase_CreateCache_Missing_details_for__0__at_line__1___ignored" xml:space="preserve"> | |||
<value>Missing details for {0} at line {1}, ignored</value> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could this warning message be rephrased so that it is more clear how big of a thing is being ignored?
Is just the line which has the missing details being ignored, or is the entire spectrum for the molecule being excluded from the spectral library?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, updated to say that the entry is being ignored
I'll have a look, thanks. I always appreciate your thoughtful reviews. |
Sometimes isotope label information can be found in InChi descriptions, e.g. the "/i1D3" in "InChI=1S/C8H8O/c1-7(9)8-5-3-2-4-6-8/h2-6H,1H3/i1D3" which tells us that three of the hydrogens in C8H8O are deuterated