It looks like you forgot to autoformat your code, we use dotnet-format: https://github.com/dotnet/format

I don't have time to review your code now, but thanks for contributing!

It looks like you forgot to autoformat your code, we use dotnet-format: https://github.com/dotnet/format

I've followed Scott's short and comprehensive guide in issue #1092 and hope formatting is now OK.

I don't have time to review your code now, but thanks for contributing!

Thank you!

Thanks for this PR @arthurits, it looks great!

I added more XML docs to try to make this plot type and its options less confusing.

I also pulled some of your code comments out of the code and am copying them here so they're easier to discuss:

Normalize on every render?

The passed values are copied into the internal array Norm, which stores the plot's data.

@swharden and @bclehmann consider if this is needed or desirable: do we want to allow the user to update its "value" array and call Render, (similarly to SignalPlot)? After all, in SignalPlot, the Update() method is used whenever the array is redimensioned but not when the array values are modified.

If so, then the Array.Copy could be left out (just keep a reference to 'values'), and move the normalization at the beginning of the Render() method.

Restating the suggestion, normalizing inside Render() would allow the user to manipulate values in the 2D array externally (similar to other plot types). It would also prevent accumulation of state in Norm, NormMax, and NormMaxes. Reducing state and moving toward static methods for calculations is almost always a good thing!

The drawback is that normalizing on every render could degrade performance, but there are probably a small number of values in these arrays so perhaps the hit is negligible. We may want to benchmark this so we can discuss it quantitatively rather than speculate about what the performance it is.

@bclehmann do you have a preference here?

This PR solves the #1086 issue so I'm going to go ahead and merge it, but if refactoring is desired related to #1091 (comment) we can make a new issue/PR to track progress on that front.

Thanks again for your help here @arthurits, and congratulations on your successful pull request! 👍 🎉 🚀

The drawback is that normalizing on every render could degrade performance

I think any performance hit here is going to be inconsequential, we normalize heatmaps with more than a million times as many values as we expect to have here very quickly. In fact, for small heatmaps (100x100 ish) we could probably eschew precomputing and get away with it, not that I would recommend it.

I think normalizing on render gives a better API and the performance impact is likely to be negligible. There is another option, make the values a property and normalize in the setter. That's a good API, and good performance, although one would keep the normalized values in the plottable.

I think normalizing on render gives a better API and the performance impact is likely to be negligible

Thanks for your input! Next time I work on this module I'll see about refactoring toward this.

make the values a property and normalize in the setter

This is possible, but I think Update() is better here because it makes it a little more obvious to the user which variables have to be changed at the same time.

Using properties users have to remember to change values and maxValues at the same time, whereas an Update() method makes it easier by tying related values together that are expected to change at the same time.

Just one more related issued. The new Update() method in RadarPlot requires the user to provide the independentAxes parameter as the method's second argument. However, the user most likely created the plot using AddRadar(values) (line 729 in Plot.Add.cs) where the independentAxes is defaulted to false.

Not sure if it's a good idea to force the user to set the independentAxes every time the plot's data values are updated, especially when he didn't set that property (the most common scenario) when he added the radar plot.

Not sure if it's a good idea to force the user to set the independentAxes every time the plot's data values are updated, especially when they didn't set that property (the most common scenario) when he added the radar plot

Good point!

That argument has a default value in Plot.AddRadar():

but not in Update():

I'll modify Update() to make that argument default to false 👍