Improve GetPointNearest support for generic signal plots by StendProg · Pull Request #886 · ScottPlot/ScottPlot

StendProg · 2021-03-18T19:34:33Z

Purpose:
PlottableSignalXY used the implementation from PlottableSignal due to inheritance, which led to inconsistent results when calling the GetPointNearestX. #809, #882
This PR brings to PlottableSignalXY it's own GetPointNearestX implementation with correct behavior.

This pull request consists of 2 commits. The very first one fixes the described problem.
In the second commit, I decided to go further, split the original interface into several, and make it so that PlottableSignals explicitly implement only what it is capable of IHasPointsXCalculatable with a single GetPointNearestX() method
On top of that, I made interfaces generic, since signals inherently support generics and this is more natural for them.

Old nongeneric IHasPoints are derived from these interfaces and are essentially unchanged. These changes should not affect the PlottableScatter in any way. But PlottableSignals lose API compatibility because they now implement a different interface, and users will have to change the interface type, although the method has remained with the same name.

If you think I went too far in the second commit, just revert it (I'm not entirely sure myself).
If you find the changes acceptable, I will be glad to have better names for the interfaces. And I would like to have time until tomorrow to once again take a fresh look at all this (I really do not want to drag out as last time, I am still ashamed).

bclehmann · 2021-03-18T19:54:34Z

Ah, I've gotten that error before. Looks like you added a public method but not documentation for it (or removed documentation from a public method).

From a quick glance it looks like it might bet this one: https://github.com/StendProg/ScottPlot/blob/201c85c0385515510aed526af71534faa9627a2e/src/ScottPlot/Plottable/SignalPlotBase.cs#L167

However running the test in debug mode will track it down if I'm wrong.

StendProg · 2021-03-18T20:12:12Z

Thanks for the tip, I spent some time trying to figure out this test.Checked all public methods, but somehow forgot about constructors. I miscalculated over time a bit, I can only continue to deal with this tomorrow.

StendProg · 2021-03-20T03:40:04Z

It looks like the problem here is completely different. On my local machine, I am getting an error test because there is no summary for SignalXY.GetPointNewestX(). This method is not even declared in SignalXY, but inherits from SignalXYGeneric. Maybe this is the case and this test does not take into account the possibility of summary inheritance. IntelliSense does an excellent job with this.
It looks like the problem is on my side, I thought that GetPointNearest inherited from PlottableSignalBase is completely hidden by the more general IHasPointsXCalculatable<TX, TY> most likely it will be so with a specific call, but at the type level, reflection sees them as different methods.

StendProg · 2021-03-20T15:45:09Z

I made private new (double x, TY, int index) GetPointNearestX(double X)
This do the trick for local tests, but online stil need documentation.
So I documented this method.
I do not have enough knowledge to understand all the nuances of hiding the interface with a more general interface. So I've tested a bit and it seems to work.

- simplify code by adding argument names - new() syntax - inline using statements

swharden · 2021-03-20T22:30:55Z

Required XML Documentation

I spent some time trying to figure out this test

Sorry this took a while to figure out! A few weeks ago I added tests to ensure plottables are documented (without requiring every module to be documented). I think the tests require public methods and constructors to have XML, but ignores public fields and properties.

I see that links to https://www.fuget.org/packages/ScottPlot now appear on official NuGet pages, so I'm hoping that better XML documentation may make the API easier to figure out just using this website. For example, the ScatterPlot API. It's nice for flat classes, but not quite as easy to navigate for complex inheritance trees.

Eventually I'd like to work toward enabling #1591 at the project level...

Generic Support for IHasPoints

Thanks for this PR! At first I was reluctant to include the second commit because those interfaces seemed pretty complex and the compiled generic lambda expression was somewhat hard to understand at first. I think I made it a little easier to read by changing some of the names, making the expression readonly, adding XML documentation, and wrapping the expression in typed functions calls so it doesn't have to be directly interacted with in the rendering/searching routines.

This extra function call may slightly impair performance, but these methods are only called from RenderLowDensity() and GetPointNearestX() so I think it is ok. Let me know if you disagree and I can refactor these to interact with the expression directly.

ScottPlot/src/ScottPlot/Plottable/SignalPlotBase.cs

Lines 167 to 188 in a1ff357

    
           /// <summary> 
        
           /// This expression adds two parameters of the generic type used by this signal plot. 
        
           /// </summary> 
        
           private readonly Func<T, T, T> AddYsGenericExpression; 
        
           /// <summary> 
        
           /// Add two Y values (of the generic type used by this signal plot) and return the result as a double 
        
           /// </summary> 
        
           private double AddYs(T y1, T y2) => Convert.ToDouble(AddYsGenericExpression(y1, y2)); 
        
           /// <summary> 
        
           /// Add two Y values (of the generic type used by this signal plot) and return the result as a the same type 
        
           /// </summary> 
        
           private T AddYsGeneric(T y1, T y2) => AddYsGenericExpression(y1, y2); 
        
           public SignalPlotBase() 
        
           { 
        
               ParameterExpression paramA = Expression.Parameter(typeof(T), "a"); 
        
               ParameterExpression paramB = Expression.Parameter(typeof(T), "b"); 
        
               BinaryExpression bodyAdd = Expression.Add(paramA, paramB); 
        
               AddYsGenericExpression = Expression.Lambda<Func<T, T, T>>(bodyAdd, paramA, paramB).Compile(); 
        
           }

OffsetX and OffsetY are broken in SignalPlotXY

I don't think this is a new bug, but I created a cookbook example to demonstrate it in this PR. It should probably be addressed in another PR (I opened an issue #890)

StendProg added 2 commits March 18, 2021 20:34

Implement GetPointNearestX for PlottableSignalXY

e1515d5

Segregate IHasPoints interface, and make it generic.

201c85c

StendProg added 5 commits March 20, 2021 10:21

Override GetPointNearestX derived from PlottableSignalBase

d1a7edd

fix formatting

ff77c20

MinRenderIndex/MaxRenderIndex support for GetPointNearestX for SignalXY

52bab84

MinRenderIndex/MaxRenderIndex support for Signal.GetPontNearestX

684ad5b

Try to fool Azure tests

210330e

swharden added 5 commits March 20, 2021 16:33

rename interface members and add XML documentation

456a6b3

improve generic add expression documentation

47cabe9

use modern language features

a1ff357

- simplify code by adding argument names - new() syntax - inline using statements

simplify OffsetY default

6981b63

add cookbook recipes demonstrating OffsetX and OffsetY

cfee860

swharden changed the title ~~SignalXY GetNearestPointX fix~~ Improve GetPointNearest support for generic signal plots Mar 20, 2021

swharden merged commit 13a12fc into ScottPlot:master Mar 20, 2021

swharden mentioned this pull request Mar 20, 2021

Incorrect IHasPoints.GetPointNearestX results using SignalPlotXY #809

Closed

swharden linked an issue Mar 20, 2021 that may be closed by this pull request

Incorrect IHasPoints.GetPointNearestX results using SignalPlotXY #809

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve GetPointNearest support for generic signal plots#886

Improve GetPointNearest support for generic signal plots#886
swharden merged 12 commits intoScottPlot:masterfrom
StendProg:SignalXYGetNearestPointXFix

StendProg commented Mar 18, 2021

Uh oh!

bclehmann commented Mar 18, 2021

Uh oh!

StendProg commented Mar 18, 2021

Uh oh!

StendProg commented Mar 20, 2021 •

edited

Loading

Uh oh!

StendProg commented Mar 20, 2021

Uh oh!

swharden commented Mar 20, 2021 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

StendProg commented Mar 18, 2021

Uh oh!

bclehmann commented Mar 18, 2021

Uh oh!

StendProg commented Mar 18, 2021

Uh oh!

StendProg commented Mar 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

StendProg commented Mar 20, 2021

Uh oh!

swharden commented Mar 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Required XML Documentation

Generic Support for IHasPoints

OffsetX and OffsetY are broken in SignalPlotXY

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

StendProg commented Mar 20, 2021 •

edited

Loading

swharden commented Mar 20, 2021 •

edited

Loading