Topic/plotter fix resample #4223

mtmccrea · 2018-12-21T23:46:52Z

Purpose and Motivation

This PR brings more support to specifying specs in Plotter, and related issues regarding GridLines and resolution.

Types of changes

This fixes a few issues regarding plotting:

Grid now follows the warping of its spec. Before the warp was assumed to be linear, so if, e.g. you assigned an exponential spec to the domain (domainSpecs_) or codomain (specs_) in Plotter, the gridlines wouldn't be at the correct locations.
Plotter resamples values if the number of values exceeded the plot width (or specified resolution). However setting the domain requires one domain value for every data point, so domain locations aren't correct when resampling. So now if the domain is set, the values won't be resampled.
- This means that if the domain is set and the plotted data set is large, refresh can be slow, but at least the values are now correct. Resampling assigned domain values is a harder problem (especially for non-linear domain specs) for a future fix.
The prResampValues method, which enforces the resolution, now also catches a change in resolution, which before wouldn't be caught unless the window was resized (wouldn't be caught on refresh either).
The prResampValues method now resamples differently. The previous approach used SequenceableCollection : resamp1, which allows floating-point resample argument resulting in a last value which isn’t the last in the array, but rather one just short of it. In plotting, this means that resampled data may appear to not reach the final value. This new approach ensures correct start and end values, and has evenly spaced (re)sampling that is at least resolution pixels.
docs were updated to provide examples and clarification on the resolution setter method.

An example of improper resampling:

//  notice the endpoint drifts when shrinking window width
p = (1, 2 .. 1000).plot.plotMode_(\linear).domainSpecs_([1, 1000, \exp].asSpec).refresh;
p.domain_((1, 2 .. 1000)/2).refresh;
// note it's a bit hard to see without gridlines working properly! (this PR fixes that too)

An example of resolution not updating:

// Changing plot resolution (x-axis)
p = 5000.collect({ |i| sinPi(i * 0.0025) }).plot;
// after setting resolution, note strange resampling,
// then resize the window and plot snaps back to original, resampled
p.resolution_(10).refresh;
p.resolution_(20).refresh;
p.resolution_(50).refresh; // undersampled
p.resolution_(1).refresh;  // default

Checklist

All previous tests are passing
- The current TestPlotter unit test only tests color setters ...
Tests were updated or created to address changes in this PR, and tests are passing
- Not sure how to make a test for this kind of (QT) thing, when changes result in display differences!
Updated documentation, if necessary
This PR is ready for review

The previous version assumed a linear distribution of gridlines, so if, e.g. a Plotter’s specs or domainSpecs were set to anything other than a linear spec, the gridlines were drawn at incorrect locations.

Resampling the data changes the number of values in the plot, and when domain is specified, there is one domain value for every original data point. So if resampling is to be supported when the domain is set, domain values will also need to be resampled.

…values. Previous used resamp1, which allows floating-point resample argument, resulting in a last value which isn’t the last in the data, but rather one just short of it. For example resampled data that’s fairly linear may appear to not reach its final value. This method ensures correct start and end values, and has evenly spaced sampling within those values that is a least ‘resolution’ pixels.

…l as resolution. And clarify resolution method description.

mtmccrea · 2019-06-13T18:42:19Z

Update: rebased onto tag-clang-format-develop

telephon · 2019-06-15T11:13:25Z

SCClassLibrary/Common/GUI/PlusGUI/Math/PlotView.sc

@@ -114,11 +116,11 @@ Plot {
 		drawGrid.vertGrid = if(gridOnY,{spec.grid},{nil});
 	}

-	draw {
+	draw { |mode = \linear|


There are many properties that are local to each Plot channel. Is there a reason that mode is special? Normally, I would tend to keep all the local properties as instance variable in the Plot object, so that we don't have to pass it around. So far, it is global, so it is in the Plotter.

If it should be local, it is more efficient to keep it in the object. But of course there are sometimes reasons you may want to draw the same plot object differently - but this doesn't seem the case here?

Thanks for giving me the opportunity to rethink this.

The goal here was to add the ability to have each plot be different mode (similar to how each plot can be a different color). When I jumped into this I tried quickly to make plotMode local, but there was a conflict with the way Plot shape-shifts when superposed—it becomes a single plot with multiple lines, not multiple plots overlaid. This meant that the plotMode would need to be passed in, or queried from Plotter (as it is with a single plotMode). I opted to be able to pass it in, assuming that there was a use case for using Plot without Plotter. This review gave me the opportunity to look closer to see that this isn't really a valid use case—Plot is only used through a Plotter—so I've modeled an update on the way plotColor is handled. See my comments in the main conversation about how that should be submitted...

telephon · 2019-06-15T11:38:08Z

SCClassLibrary/Common/GUI/PlusGUI/Math/PlotView.sc

 		var ycoord = this.dataCoordinates;
 		var xcoord = this.domainCoordinates(ycoord.size);
+		mode = (mode ?? plotter.plotMode).asArray;


this would then just be the instance variable of the Plot object.

As described above, because there was originally no instance variable for this in Plot, in an attempt to keep it global and free the dependency on Plotter, I opted to pass this in from outside. I've rethought this though, see more following...

telephon · 2019-06-15T11:39:25Z

SCClassLibrary/Common/GUI/PlusGUI/Math/PlotView.sc

@@ -201,8 +204,8 @@ Plot {
 				Pen.beginPath;
 				Pen.strokeColor = plotColor.at(0);
 				Pen.fillColor= plotColor.at(0);
-				this.perform(mode, xcoord, ycoord);
-				if (this.needsPenFill(mode)) {
+				this.perform(mode.at(0), xcoord, ycoord);


mode.at(0) seems not correct if the mode of each plot is different.

Maybe you intended that there could be different modes for each channel?

Yes, the new feature here is to add the ability for multiple plot modes, as you can have multiple plot colors. When Plotter is not superposed, each Plot has only one plot mode, whereas when superpose is true the plot mode is an array of modes. IIC, I've redesigned this. See the main thread...

mtmccrea · 2019-06-17T17:03:50Z

@telephon, thanks for getting the review underway! I'll be following up shortly. It's been a while since I started this PR so I'll need to review it myself!

mtmccrea · 2019-06-18T06:07:26Z

OK, I've had a chance to review this and you raised good points that gave me the opportunity to redesign the behavior of plotMode this, based on the way plotColor(s) is handled.

To digress a bit, when I revisited this PR recently, I hastily assumed this was actually a different PR in the same part of the code (I forgot this other one had already been merged). This plotMode change would have been more at home in the other PR, not this one.

So now that I've revised the change, I'm thinking it may be better to roll back this PR to 64081f5 to confine it on the resampling topic, and submit this new plotMode change as a new PR. Does that that sounds reasonable?

mtmccrea · 2019-06-19T00:59:29Z

After a bit of research I found rolling back wasn't going to be as big of a problem as I thought, so I've done that to limited this PR to the scope of the original description. I'll put all things related to plotMode in a new PR #4459.
Thanks for your patience and sorry for the noise, @telephon!

telephon

thank you!

mtmccrea · 2019-07-01T01:07:59Z

~~Hi @telephon, does the Reviewer handle merging or should I do that? In the past it's been a mix I think.~~

scratch that, I see this has the 3.11 milestone.

dyfer

Everything looks good to me. Grids reflect ControlSpec properly now, resample works as intended.

mossheim added comp: help schelp documentation comp: class library SC class library labels Jan 8, 2019

patrickdupuis added this to the 3.11 milestone Jan 26, 2019

mtmccrea added 4 commits June 13, 2019 11:38

Grid: grid lines now follow their assigned specs

ea59291

The previous version assumed a linear distribution of gridlines, so if, e.g. a Plotter’s specs or domainSpecs were set to anything other than a linear spec, the gridlines were drawn at incorrect locations.

Docs: Plotter - add examples for setting specs and domainSpecs as wel…

6280aaf

…l as resolution. And clarify resolution method description.

mtmccrea force-pushed the topic/plotter-fix-resample branch from eccc665 to 6280aaf Compare June 13, 2019 18:41

PlotView: -asInt -> -asInteger

64081f5

telephon reviewed Jun 15, 2019

View reviewed changes

mtmccrea force-pushed the topic/plotter-fix-resample branch from 9ab7d22 to 64081f5 Compare June 19, 2019 00:52

telephon approved these changes Jun 19, 2019

View reviewed changes

mtmccrea mentioned this pull request Jul 29, 2019

[classlib] Plotter: correct resampling of domain given fixed Array:series method. #4510

Merged

3 tasks

joshpar assigned dyfer Sep 22, 2019

dyfer self-requested a review September 27, 2019 01:17

dyfer approved these changes Sep 27, 2019

View reviewed changes

muellmusik merged commit d0e3c01 into supercollider:develop Dec 27, 2019

dyfer mentioned this pull request Sep 1, 2020

DrawGrid etc now plots log grids, but incorrectly #5156

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Topic/plotter fix resample #4223

Topic/plotter fix resample #4223

mtmccrea commented Dec 21, 2018 •

edited

Loading

mtmccrea commented Jun 13, 2019

telephon Jun 15, 2019 •

edited

Loading

mtmccrea Jun 18, 2019 •

edited

Loading

telephon Jun 15, 2019

mtmccrea Jun 18, 2019

telephon Jun 15, 2019 •

edited

Loading

mtmccrea Jun 18, 2019

mtmccrea commented Jun 17, 2019

mtmccrea commented Jun 18, 2019

mtmccrea commented Jun 19, 2019 •

edited

Loading

telephon left a comment

mtmccrea commented Jul 1, 2019 •

edited

Loading

dyfer left a comment

Topic/plotter fix resample #4223

Topic/plotter fix resample #4223

Conversation

mtmccrea commented Dec 21, 2018 • edited Loading

Purpose and Motivation

Types of changes

Checklist

mtmccrea commented Jun 13, 2019

telephon Jun 15, 2019 • edited Loading

Choose a reason for hiding this comment

mtmccrea Jun 18, 2019 • edited Loading

Choose a reason for hiding this comment

telephon Jun 15, 2019

Choose a reason for hiding this comment

mtmccrea Jun 18, 2019

Choose a reason for hiding this comment

telephon Jun 15, 2019 • edited Loading

Choose a reason for hiding this comment

mtmccrea Jun 18, 2019

Choose a reason for hiding this comment

mtmccrea commented Jun 17, 2019

mtmccrea commented Jun 18, 2019

mtmccrea commented Jun 19, 2019 • edited Loading

telephon left a comment

Choose a reason for hiding this comment

mtmccrea commented Jul 1, 2019 • edited Loading

dyfer left a comment

Choose a reason for hiding this comment

mtmccrea commented Dec 21, 2018 •

edited

Loading

telephon Jun 15, 2019 •

edited

Loading

mtmccrea Jun 18, 2019 •

edited

Loading

telephon Jun 15, 2019 •

edited

Loading

mtmccrea commented Jun 19, 2019 •

edited

Loading

mtmccrea commented Jul 1, 2019 •

edited

Loading