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

Interpolation for line charts #1139

Merged
merged 46 commits into from
Mar 15, 2021
Merged

Interpolation for line charts #1139

merged 46 commits into from
Mar 15, 2021

Conversation

ClumsyPenguin
Copy link
Contributor

@ClumsyPenguin ClumsyPenguin commented Mar 9, 2021

NOTE: this code is not finished, this PR is merely for looking into my issue that i am running into so you guys gets a better insight of what i am doing wrong at the moment....

The switch statement in Line.Razor needs be cleaned up, etc.

@mikes-gh mikes-gh marked this pull request as draft March 9, 2021 09:21
@ClumsyPenguin
Copy link
Contributor Author

I really want to refactor the code from line 127 in Line.razor.cs.

This should fix the underlying problem on how the grid is builded in the class.
I want to refactor it, but need advice on which route you guys prefer to see.

For now the interpolation mechanism just works.

@ClumsyPenguin ClumsyPenguin marked this pull request as ready for review March 9, 2021 16:47
@just-the-benno
Copy link
Contributor

As a proposal:

Add an interface like ILineInterpolator with all the public properties of your SplineInterpolator and in addition a property like InterpolationRequired. The SplineInterpolator implements this new interface and returns always true for the new property. While a newly created class called for instance NoInterpolation would return false. This is needed to model the Straight case.

So, your switch then just initiated the right classes (based on the enum) and after the switch, you have an if statement like

if(interpolator.InterpolationRequired == true)
{
       horizontalSpace = (boundWidth - horizontalStartSpace - horizontalEndSpace) / interpolator.interpolatedXs.Length;
                        foreach (var yValue in interpolator.interpolatedYs)
                        {

                            if (firstTime)
                            {

                                chartLine += "M ";
                                firstTime = false;
                                gridValueX = horizontalStartSpace;
                                gridValueY = verticalStartSpace;
                            }
                            else
                            {
                                chartLine += " L ";
                                gridValueX += horizontalSpace;
                                gridValueY = verticalStartSpace;
                            }
                            gridValueY = yValue;
                            chartLine = chartLine + ToS(gridValueX) + " " + ToS(gridValueY);
} else 
{
    IsChartSplined = false;
}

Does it help?

@ClumsyPenguin
Copy link
Contributor Author

ClumsyPenguin commented Mar 9, 2021

Awesome suggestion, yeah i could solve it a infinite different ways, but i don't really know the "design" you guys like me to implement.

But your suggestions looks something of what i had in mind.

I will push an update later this evening of tomorrow.

@ClumsyPenguin
Copy link
Contributor Author

ClumsyPenguin commented Mar 9, 2021

@just-the-benno

var chartLine = "";
double gridValueX = 0;
double gridValueY = 0;
var firstTime = true;
double[] XValues = new double[item.Data.Length];
double[] YValues = new double[item.Data.Length];
SplineInterpolator interpolator;
for (var i = 0; i <= item.Data.Length - 1; i++)
{
    if (i == 0)
        XValues[i] = 30;
    else
        XValues[i] = XValues[i - 1] + horizontalSpace;

    var gridValue = (item.Data[i]) * verticalSpace / gridYUnits;
    YValues[i] = boundHeight - (verticalStartSpace + gridValue);

}
switch (curveEnum)
{
    case CurveEnum.NaturalSpline:            
        interpolator = new NaturalSpline(XValues, YValues);
        break;
    case CurveEnum.EndSlope:
        interpolator = new EndSlopeSpline(XValues, YValues);                     
        break;
    case CurveEnum.Periodic:
        interpolator = new PeriodicSpline(XValues, YValues);                      
        break;
    case CurveEnum.Straight:
        IsChartSplined = false;
        break;
    default:
        IsChartSplined = false;
        break;
}

if (interpolator.InterpolationRequired == true)
{
    horizontalSpace = (boundWidth - horizontalStartSpace - horizontalEndSpace) / interpolator.interpolatedXs.Length;
    foreach (var yValue in interpolator.interpolatedYs)
    {

        if (firstTime)
        {

            chartLine += "M ";
            firstTime = false;
            gridValueX = horizontalStartSpace;
            gridValueY = verticalStartSpace;
        }
        else
        {
            chartLine += " L ";
            gridValueX += horizontalSpace;
            gridValueY = verticalStartSpace;
        }
        gridValueY = yValue;
        chartLine = chartLine + ToS(gridValueX) + " " + ToS(gridValueY);
    }
}
else
{
    IsChartSplined = false;
}

With this snippet i get an Use of unassigned local variable 'interpolator'. Obviously because i don't make an interpolation implementation in every switch case how can i quickly get around that.

EDIT: Realised i answered my own question 😄

@just-the-benno
Copy link
Contributor

just-the-benno commented Mar 9, 2021

😄

There is still a tiny mistake in it

    case CurveEnum.Straight:
        IsChartSplined = false;
        break;
    default:
        IsChartSplined = false;
        break;
        

In this cases interpolator is null and the if statement will fail. Either you add the ? operator or check explicit for null

if (interpolator != null && interpolator.InterpolationRequired == true)
{
...
}

And you could summarize both cases

   case CurveEnum.Straight:
   default:
       IsChartSplined = false;
       break;
       

@just-the-benno
Copy link
Contributor

There are two things I want to mention

@ClumsyPenguin Should there be unit tests of the interpolation to make sure that it works, especially for edge cases like rectangle-shaped series or very dense ones?

Regarding the options
Currently, I'm working on a new feature branch of "enhanced" charts with a completely new configuration model because the current one has some limitations. For example, I'd argue that interpolation shouldn't be exclusively bound to the chart but ultimately to a series. There are cases where it is useful to show not interpolated and interpolated lines in the same chart or where different algorithms should be used for each series.

I'd recommend using @ClumsyPenguin approach for now and discuss the option paradigm when I finished my work.

@ClumsyPenguin
Copy link
Contributor Author

@just-the-benno For now interpolation is for line charts only, so there is no use for it on a bar chart or pie/donuts.
But some unit tests regarding this new functionality is indeed a must i overlooked.

@Garderoben Garderoben merged commit 23e1163 into MudBlazor:feature/charts Mar 15, 2021
HClausing pushed a commit to HClausing/MudBlazor that referenced this pull request Mar 23, 2021
* added interpolation mechanism

* commit to continue work on laptop

* Cache bust 5.0.5

* first interpolation implementation

* spliced code up to reuse and add in documentation

* Add support for Required parameter to Select component (MudBlazor#1127)

* Add key to MudDialogInstance (MudBlazor#1137)

* Fix MudBlazor#1103 Arithmetic overflow in chart examples

* Fix MudBlazor#1144 Scroll to top example

* fixed initial Y value

* made splined width 100%

* added menu for all the different interpolation algorithms

* Added some base position/visibility classes.

* big cleanup

* cleanup notinterpolation constructor

* Docs: ScrollToTop removed inline <style> css from examples.

* quickly added latest suggestions

* Removed svg height from base scss.

* add action buttons to pickers

* add clear option to pickers

* renamed curveEnum to Interpolationoptions

* change drawer-content height to 100%

* fix MudBlazor#1153

* fix MudBlazor#994

* Docs: Removed docs specific component.

* Add application insights to website

* Add application insights to website

* fix MudBlazor#1169

* Add test today should be selected

* fix appbar/maincontent css in firefox esr

* Fix flaky async autocomplete test

* Fire onclick event in MudNavLink

* fix spelling error

* minor grammar fixes

* improve title grammar

* include subtitle  and description properties for the header

* add information regarding material design surfaces

* minor grammar and punctuation fixes

* Fixed Typo in FlexPage.razor

* MudSwitch removed Class from the internal switch class, Fix for MudBlazor#1163
HClausing pushed a commit to HClausing/MudBlazor that referenced this pull request Apr 5, 2021
* added interpolation mechanism

* commit to continue work on laptop

* Cache bust 5.0.5

* first interpolation implementation

* spliced code up to reuse and add in documentation

* Add support for Required parameter to Select component (MudBlazor#1127)

* Add key to MudDialogInstance (MudBlazor#1137)

* Fix MudBlazor#1103 Arithmetic overflow in chart examples

* Fix MudBlazor#1144 Scroll to top example

* fixed initial Y value

* made splined width 100%

* added menu for all the different interpolation algorithms

* Added some base position/visibility classes.

* big cleanup

* cleanup notinterpolation constructor

* Docs: ScrollToTop removed inline <style> css from examples.

* quickly added latest suggestions

* Removed svg height from base scss.

* add action buttons to pickers

* add clear option to pickers

* renamed curveEnum to Interpolationoptions

* change drawer-content height to 100%

* fix MudBlazor#1153

* fix MudBlazor#994

* Docs: Removed docs specific component.

* Add application insights to website

* Add application insights to website

* fix MudBlazor#1169

* Add test today should be selected

* fix appbar/maincontent css in firefox esr

* Fix flaky async autocomplete test

* Fire onclick event in MudNavLink

* fix spelling error

* minor grammar fixes

* improve title grammar

* include subtitle  and description properties for the header

* add information regarding material design surfaces

* minor grammar and punctuation fixes

* Fixed Typo in FlexPage.razor

* MudSwitch removed Class from the internal switch class, Fix for MudBlazor#1163
@ScarletKuro
Copy link
Member

Hi @ClumsyPenguin! I hope you're doing well.

As the author of the charts, I have a question for you. While I understand that you may not recall all the specifics, there is a method in the codebase that caught my attention:

public double Integrate()
{
    double integral = 0;
    for (int i = 0; i < h.Length; i++)
    {
        double termA = a[i] * h[i];
        double termB = b[i] * Math.Pow(h[i], 2) / 2.0;
        double termC = c[i] * Math.Pow(h[i], 3) / 3.0;
        double termD = d[i] * Math.Pow(h[i], 4) / 4.0;
        integral += termA + termB + termC + termD;
    }
    return integral;
}

I noticed that the classes EndSlopeSpline, NaturalSpline, and PeriodicSpline invoke this method in their constructors. However, they don't seem to utilize the calculated value, and the method itself doesn't modify any local properties. Could this be considered a bug, or is it possible that this method can be safely removed? I found this in the PVS report (Issue 1): https://pvs-studio.com/en/blog/posts/csharp/1032/

I appreciate your insights on this matter. Thank you!

@ClumsyPenguin
Copy link
Contributor Author

ClumsyPenguin commented Jun 25, 2023

Hi @ScarletKuro

Thank you for your question. This is something i wrote in 2021. When i was practically a junior developer, so i don't really remember why i wrote this per se, so i did some backtracking on the potential 'why'.
However this is what found after some research:
Once you've computed the value from the Integrate() method, you can store it and use it for any later calculations or comparisons. I see that this implementation is not really finished correctly, the value should be exposed or the method to be deleted completely...

To give you some potential use cases for instance, if you are studying several different curves or datasets, you might use the integral (the area under the curve) to compare them. In physical phenomena, the integral of a curve can represent a total amount of a quantity over a certain period (like total displacement, total energy, total probability, etc.).

Remember that this value represents the approximate integral of the function over the range of your x values. You should interpret and use this value in the context of what your x and y values represent.

This can be extended so you could pick a resolution or a sub-set of the available x values and achieve a more granular control of this calculation.

I hope this answered your question well.

EDIT: To give you some additional context, when i was extending MudBlazor at the time on these charts. I was working at a company that was specialized in breweries and chemical industry software. The so-called MES Systems...
We ended up forking a version of MudBlazor and potentially extending the behavior in our own fork. I think it was uses to measure total throughput of brewing capacity and so-on, although i cannot confirm this, since i no longer work for that company.

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

8 participants