Library spectrum match graph tooltip implementation #3210

rita-gwen · 2024-11-01T00:38:37Z

No description provided.

…github.com/ProteoWizard/pwiz into Skyline/work/20241021_GraphSpectrumTooltip

nickshulman

Tooltips look nice.

New properties on the property sheet also look nice.

Here are some cosmetic comments about the code.

nickshulman · 2024-11-20T23:36:11Z

pwiz_tools/Skyline/Controls/Graphs/GraphSpectrum.cs

+
+            public Size RenderTip(Graphics g, Size sizeMax, bool draw)
+            {
+                if (HasTip)


When the entire body of a function is inside of an if statement, I usually try to arrange things to reduce the amount that the code has to be indented.
That would be:

if (!HasTip) { return Size.Empty; } // rest of the body of the function

nickshulman · 2025-07-30T06:08:23Z

pwiz_tools/Shared/zedgraph/ZedGraph/GraphPane.cs

+            }
+            return false;
+        }
+        public static int Between<T>(T x, T low, T high) where T : IComparable<T>


I would recommend getting rid of this method.
The problem with implementing a method "Between" is that the order of parameters and return value are not something that is universally agreed upon.
The natural ordering of arguments is "low, middle, high", but a case can also be made for the "middle, low, high" that you have here.

nickshulman · 2025-07-30T06:09:09Z

pwiz_tools/Shared/zedgraph/ZedGraph/GraphPane.cs

+                            if (xVal != PointPair.Missing &&
+                                yVal != PointPair.Missing)
+                            {
+                                if (Between(yAct, 0, yVal) != 0)


Is this equivalent to if (yAct < 0 || yAct > yVal)?

nickshulman · 2025-07-30T06:24:03Z

pwiz_tools/Skyline/Controls/Graphs/GraphsResources.resx

    <value>Failure loading spectrum. Library may be corrupted.</value>
  </data>
+  <data name="GraphSpectrum_ToolTip_mz">
+    <value>Observed M/z:</value>


The "m" and the "z" must have the same case as each other.

In the Document Grid, "M/Z" is written in upper case, but I'm not sure that's correct.
I see that many menu items have it in lowercase.
I'm not sure whether this should be "Observed M/Z:" or "Observed m/z:"

nickshulman · 2025-07-30T06:24:18Z

pwiz_tools/Skyline/Controls/Graphs/GraphsResources.resx

    <value>No quantitative chromatograms found</value>
  </data>
+  <data name="ToolTipImplementation_RenderTip_Calculated_Mass" xml:space="preserve">
+    <value>Calculated M/z</value>


However, I wonder this should be "Ion m/z" because the context menu item which causes these numbers to be shown on the graph is "Ion m/z Values"

nickshulman · 2025-07-30T06:26:28Z

pwiz_tools/Skyline/Controls/Graphs/SpectrumGraphItem.cs

+            var massError = mfi.PredictedMz - rmi.ObservedMz;
+            massError = SequenceMassCalc.GetPpm(mfi.PredictedMz, massError);
+            massError = Math.Round(massError, 1);
+            sb.Append(string.Format(Resources.GraphSpectrum_MassErrorFormat_ppm, (massError > 0 ? @"+" : string.Empty), massError));


The StringBuilder here is unnecessary.
This could just be:
return string.Format(Resources.GraphSpectrum_MassErrorFormat_ppm, (massError > 0 ? @"+" : string.Empty), massError);

nickshulman · 2025-07-30T06:36:59Z

pwiz_tools/Skyline/Model/SpectrumPropertiesResx.resx

    <value>Number of measured spectra for this peptide in the library</value>
  </data>
+  <data name="Description_PeakCount">
+    <value>Number of non-zero peaks in the library spectrum</value>


I don't think "non-zero peaks" means anything (specifically, there's no such thing as a zero peak).

I might say "Number of m/z values in the spectrum that had non-zero intensity", but I also might be wrong.

nickshulman · 2025-07-30T06:44:22Z

pwiz_tools/Shared/zedgraph/ZedGraph/GraphPane.cs

+
+            double yPixPerUnitAct, yAct, yMinAct, yMaxAct, xAct;
+            double minDist = 1e20;
+            double xVal, yVal, dist = 99999, distX, distY;


dist, distX and distY are not used.

By default, ReSharper does not perform code analysis on the files in the ZedGraph project but you can right-click on the pause button in the top right corner of the code editor and choose "Resume Analysis"

There are quite a few unused local variables in this function.

The original ZedGraph code has too many code inspection problem, so I usually keep it disabled there. I cleaned that FindNearestStick method from the unused variables, though.

…github.com/ProteoWizard/pwiz into Skyline/work/20241021_GraphSpectrumTooltip

nickshulman · 2025-08-05T22:46:05Z

pwiz_tools/Shared/zedgraph/ZedGraph/GraphPane.cs

+            }
+            return false;
+        }
+        private double LogScalePixelDistance(double positionA, double positionB, double totalDistance)


I know this isn't part of this pull request, but I don't trust the math in this "LogScalePixelDistance".

Typically, the way to figure out the pixel distance between two coordinate values is to call "axis.Scale.Transform" on both of them and subtract the two results. That works for all types of scales.

Yes, your intuition was right, as usual. That function returned incorrect values. Removed.

nickshulman · 2025-08-05T23:06:11Z

pwiz_tools/Skyline/Controls/Graphs/GraphSpectrum.cs

                                : mirrorSpectrum.SpectrumPeaksInfo;
-                            mirrorGraphItem = MakeGraphItem(mirrorSpectrum, selection, settings, peaksInfo);
-                            mirrorGraphItem.Invert = true;
+                            MirrorGraphItem = MakeGraphItem(mirrorSpectrum, selection, settings, peaksInfo);


The local variable mirrorGraphItem is still being used in a few places in this function and causes NullReferenceException's.
Those need to be changed to the capital MirrorGraphItem

nickshulman · 2025-08-05T23:07:13Z

pwiz_tools/Skyline/Controls/Graphs/SpectrumGraphItem.cs

-        private string GetLabel(MatchedFragmentIon mfi, int rank, bool showMz)
+        public static string GetMassErrorString(LibraryRankedSpectrumInfo.RankedMI rmi, MatchedFragmentIon mfi)
+        {
+            var sb = new StringBuilder();


This StringBuilder is no longer used.

…github.com/ProteoWizard/pwiz into Skyline/work/20241021_GraphSpectrumTooltip

rita-gwen · 2025-08-13T19:42:07Z

Any other comments before I merge this?

brendanx67 · 2025-08-13T19:52:50Z

pwiz_tools/Skyline/Skyline.csproj

-    <Compile Include="Alerts\ArdiaLogoutDlg.cs">
-      <SubType>Form</SubType>
-    </Compile>
+    <Compile Include="Alerts\ArdiaLogoutDlg.cs" />


This file looks like all changes that are not relevant to your PR.

brendanx67 · 2025-08-13T19:54:49Z

pwiz_tools/Skyline/TestTutorial/TargetedMSMSTutorialTest.cs


        private void TestLibraryMatchPropertySheet()
        {
+            //TODO: [RC] Fix tests, add some tooltip testing code


Is there any testing of this new code? Would prefer to see this removed before you merge, and ideally clear new testing added. Please run code coverage and make sure your new code gets hit and covered well during some test run.

Added the testing code and run the code coverage. Looks like the new tooltip code is exercised well.

nickshulman

Looks good to me.

nickshulman · 2025-08-26T21:27:28Z

pwiz_tools/Skyline/TestTutorial/TargetedMSMSTutorialTest.cs

+            // 191, 95; 169, 175
+            var testData = new Dictionary<Point, string>()
+            {
+                {new Point(191, 95), "Observed m/z:\t951.6229\nIntensity:\t3814516\nRank:\t1\nMatched Ions\tIon m/z (calculated)\ny8\t951.4782  -152.1 ppm"},


This test seems to fail in French ~~, Chinese and Japanese~~.

Thank you, Nick. I think it's the numbers formatting.

…github.com/ProteoWizard/pwiz into Skyline/work/20241021_GraphSpectrumTooltip

Library spectrum match graph tooltip implementation

8e89669

rita-gwen requested review from brendanx67 and nickshulman November 4, 2024 19:18

rita-gwen and others added 11 commits November 5, 2024 13:45

Code inspection fixes

dfbb4e1

Manual merge from master

ba11afe

Merge branch 'master' into Skyline/work/20241021_GraphSpectrumTooltip

2beb44e

Merge branch 'master' into Skyline/work/20241021_GraphSpectrumTooltip

38cd187

Merge branch 'master' into Skyline/work/20241021_GraphSpectrumTooltip

11f7a14

Large merge from master

98abbff

Bad merge leftovers

58c7ad1

Show tolltip over a label. Formatting fixes as discussed.

b7214ba

Merge branch 'Skyline/work/20241021_GraphSpectrumTooltip' of https://…

7345e44

…github.com/ProteoWizard/pwiz into Skyline/work/20241021_GraphSpectrumTooltip

Merge branch 'master' into Skyline/work/20241021_GraphSpectrumTooltip

f7b71eb

Merge branch 'master' into Skyline/work/20241021_GraphSpectrumTooltip

de8f404

nickshulman reviewed Jul 30, 2025

View reviewed changes

rita-gwen and others added 4 commits August 4, 2025 17:35

Code review fixes

6944e2a

Merge branch 'Skyline/work/20241021_GraphSpectrumTooltip' of https://…

80a0b09

…github.com/ProteoWizard/pwiz into Skyline/work/20241021_GraphSpectrumTooltip

Merge branch 'master' into Skyline/work/20241021_GraphSpectrumTooltip

c58ea2e

Merge branch 'master' into Skyline/work/20241021_GraphSpectrumTooltip

4515bf0

nickshulman reviewed Aug 5, 2025

View reviewed changes

rita-gwen added 2 commits August 12, 2025 18:11

Code review and test fixes

9d9c439

Merge branch 'Skyline/work/20241021_GraphSpectrumTooltip' of https://…

5e489f5

…github.com/ProteoWizard/pwiz into Skyline/work/20241021_GraphSpectrumTooltip

brendanx67 requested changes Aug 13, 2025

View reviewed changes

nickshulman approved these changes Aug 19, 2025

View reviewed changes

rita-gwen added 2 commits August 26, 2025 14:07

Test code added and unwanted changes rolled back

31d3cb8

Merge branch 'master' into Skyline/work/20241021_GraphSpectrumTooltip

b59b13d

nickshulman reviewed Aug 26, 2025

View reviewed changes

rita-gwen added 2 commits August 26, 2025 16:53

Fixed test localization issue

73e967d

Merge branch 'Skyline/work/20241021_GraphSpectrumTooltip' of https://…

2cf120e

…github.com/ProteoWizard/pwiz into Skyline/work/20241021_GraphSpectrumTooltip

rita-gwen added 2 commits August 26, 2025 16:55

Merge branch 'master' into Skyline/work/20241021_GraphSpectrumTooltip

e34caf2

Merge branch 'master' into Skyline/work/20241021_GraphSpectrumTooltip

142275a

Library spectrum match graph tooltip implementation #3210

Are you sure you want to change the base?

Library spectrum match graph tooltip implementation #3210

Uh oh!

Conversation

rita-gwen commented Nov 1, 2024

Uh oh!

nickshulman left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rita-gwen commented Aug 13, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nickshulman left a comment

Choose a reason for hiding this comment

Uh oh!

nickshulman Aug 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

nickshulman Aug 26, 2025 •

edited

Loading