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

Special ions annotations. #2114

Merged
merged 21 commits into from Aug 22, 2022
Merged

Conversation

rita-gwen
Copy link
Contributor

No description provided.

Copy link
Contributor

@brendanx67 brendanx67 left a comment

Choose a reason for hiding this comment

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

Looks mostly okay. Needs a few changes to merge to the project. And I would like to see a demo.

@@ -193,7 +193,9 @@ protected override void DoTest()
Assert.AreEqual(testRange, SkylineWindow.GraphFullScan.Range);

RunUI(() => SkylineWindow.SynchMzScale(SkylineWindow.GraphSpectrum, false)); // Sync from the library match to the full scan viewer
TestSpecialIonsAnnotations();
//annotations are not shown in the offscreen mode
if (!Skyline.Program.SkylineOffscreen)
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems overly drastic. If it is a painting issue, then you should add a method that tells what would be annotated during a paint, and use that to make sure the graph understands what it is supposed to do. We use this approach a lot with our graphs.

@@ -1,513 +1,520 @@
/*
Copy link
Contributor

Choose a reason for hiding this comment

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

Avoid diffs like this of the entire file. Likely a change in line endings.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure it's a real issue. I cannot identify a commit that introduced this whole file change. When I look at the changes in each individual commit in this PR they all show only individual line-by-line changes. When I checkout the original file and compare with it I do not see any global differences either. Both files have CR/LF line endings. How do we debug such things?

moleculeMasses = new MoleculeMasses(
SequenceMassCalc.GetMZ(calcMatchPre.GetPrecursorMass(Sequence), PrecursorAdduct),
new IonMasses(calcMatch.GetPrecursorFragmentMass(Sequence),
calcMatch.GetFragmentIonMasses(Sequence)));
new IonMasses(calcMatch.GetPrecursorFragmentMass(Sequence), ionTypes) .ChangeKnownFragments(specialIons)
Copy link
Contributor

Choose a reason for hiding this comment

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

Wierd spacing.

@@ -701,6 +719,10 @@ private RankedMI CalculateRank(RankingState rankingState, RankedMI rankedMI)
continue;
}

//custom ions have been already matched above. No need to do anything.
Copy link
Contributor

Choose a reason for hiding this comment

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

Your comment style is at odds with the project. Put a space after the // and capitalize the first letter.

@@ -1,2821 +1,2831 @@

Copy link
Contributor

Choose a reason for hiding this comment

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

Avoid whole file diffs, likely the result of line endings changing.

@@ -914,7 +918,7 @@ public void ShowLosses(IEnumerable<string> losses)
private void IonTypeSelector_IonTypeChanges(IonType type, bool show)
{
switch (type)
{
{
Copy link
Contributor

Choose a reason for hiding this comment

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

This brace indenting change seems unnecessary and incorrect.

{
ShowSpecialIons(!_graphSpectrumSettings.ShowSpecialIons);
}

public void SynchMzScaleToolStripMenuItemClick(IMzScalePlot source = null)
{
if(ListMzScaleCopyables().Count() < 2)
Copy link
Contributor

Choose a reason for hiding this comment

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

If statements get spaces between if and (. They are not function calls.

@@ -984,9 +993,9 @@ private void synchMzScaleToolStripMenuItem_Click(object sender, EventArgs e)
}

//Testing support
Copy link
Contributor

Choose a reason for hiding this comment

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

Again put a space between // and the next character.

@@ -995,7 +1004,7 @@ public void chargesMenuItem_DropDownOpening(object sender, EventArgs e)
if (chargesContextMenuItem.DropDownItems.Count > 0 && chargesContextMenuItem.DropDownItems[0] is MenuControl<ChargeSelectionPanel> chargeSelector)
{
chargeSelector.Update(_graphSpectrumSettings, DocumentUI.Settings.PeptideSettings);
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This indenting change seems incorrect.

@rita-gwen rita-gwen merged commit 0333b7c into master Aug 22, 2022
@brendanx67 brendanx67 deleted the Skyline/work/20220505_specialIonAnnotations branch August 23, 2022 15:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants