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

Chronologer Retention Time model predictor #761

Open
wants to merge 109 commits into
base: master
Choose a base branch
from

Conversation

elaboy
Copy link
Contributor

@elaboy elaboy commented Feb 13, 2024

This PR adds the Chronologer model and the weights from the article.

Proteomics csporj file now contains a conditional installing statement for the operating system, this is to enable CUDA for compatible OS

MICHAEL SHORTREED and others added 30 commits November 18, 2021 12:30
Copy link
Contributor

@trishorts trishorts left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please check my comments

/// The base sequence is the sequence without modifications and the full peptide sequence is the sequence with modifications.
/// The model is intended to be used with sequences of length 50 or less, and only supports modifications present in the Chronologer dictionary.
/// Sequences not supported by chronologer will return null.
/// </summary>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add example full sequence to tensor

@elaboy elaboy requested review from trishorts and nbollis September 3, 2024 19:25
}

if (baseSequence.Length <= 50) //Chronologer only takes sequences of length 50 or less
{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If statement is unnecessary due to above check. that will give you coverage of lines 222-223

{ ('S', "Phosphorylation on S"), 25 }, //'Phosphoserine
{ ('T', "Phosphorylation on T"), 26 }, //'Phosphothreonine
{ ('Y', "Phosphorylation on Y"), 27 }, //'Phosphotyrosine
{ ('K', "Accetylation on K"), 28 }, //acetylation
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about alternative spellings? Akin to the issue from Zhuoxin's two phosph on S bars in her presentation. Unimod may say: Phospho on S where we may say Phosphorylation on S

mzLib/TestFlashLFQ/TestFlashLFQ.cs Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are we downgrading easy.common?

namespace Omics.Fragmentation.Peptide
{
{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

extra space

catch
{
Console.WriteLine(new Exception("This device does not support CUDA, using CPU"));
device = new torch.Device(DeviceType.CPU);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a way to do this check without catching an exception?

{
preds[outputIndex] = predictionHolder[outputIndex];
});

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what does the "preds" array do here? It doesn't look like it's used

var baseSeq = baseSequences[i];
var fullSeq = fullSequences[i];

var (tensor, compatible) = Tensorize(baseSeq, fullSeq, out bool chronologerCompatible);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what is the difference between compatible and chronologerCompatible? Why is one checked but not the other?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants