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

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

Merged
merged 1 commit into from
Aug 28, 2019

Conversation

mtmccrea
Copy link
Member

@mtmccrea mtmccrea commented Jul 29, 2019

Purpose and Motivation

Fixes #4505, which came up as an issue after #4454 broke #4082.

The way the plot's domain values were previously calculated, accumulated resolution error meant that Array:series prevented the domain from reaching its upper boundary in some cases (before #4454, Array:series would overshoot its end value), which in turn meant the domain array would be one value less than the data coordinate array.

This fix replace Array:series with Array:interpolation to resample the domain, ensuring the domain hits the last value and its size matches exactly with the data coordinate array.

An example of the difference between the two methods:

(
var n = 228; // not all values of n "break"

y = n.collect{rrand(0,1.0)};

// generate corresponding domain values, 0 > 1
// via Array.series
x = (0.0, 0.0 + (y.size - 1).reciprocal .. 1.0);
"Array.series\n\ty size: %, x size: %, x last: %\n".postf(y.size, x.size, x.last);
// via Array.interpolation
x = Array.interpolation(y.size, 0.0, 1.0);
"Array.interpolation\n\ty size: %, x size: %, x last: %\n".postf(y.size, x.size, x.last);
\ok
)

Types of changes

  • Bug fix

To-do list

  • Code is tested
  • All tests are passing
    - [ ] Updated documentation
  • This PR is ready for review

@mtmccrea
Copy link
Member Author

Related:

#4223, and #4459, are closely tied to this part of the code (Plotter), and have already been reviewed and approved, and have a 3.11 milestone, and no milestone, respectively. Could we consider rolling these into 3.10.3? #4459 in particular rolls back changes introduced in #4082, as detailed here, so it would be good to have them go out together.

@mtmccrea
Copy link
Member Author

and thanks, @snappizz for reporting the issue!

Copy link
Contributor

@nhthn nhthn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

jeez sorry it took me so long to get around to this. looks good, thanks!

@nhthn nhthn added this to the 3.10.3 milestone Aug 25, 2019
@nhthn
Copy link
Contributor

nhthn commented Aug 27, 2019

fyi i made a stupid mistake -- i put #4511 on the release board when i really meant to do this one. this one's more critical since it's a bug introduced in 3.10.3. thanks @mtmccrea for the help, and again my apologies for being real flaky on this one

@mtmccrea
Copy link
Member Author

Thanks @snappizz for the follow up!

@nhthn nhthn added the comp: class library SC class library label Aug 28, 2019
Copy link
Member

@joshpar joshpar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. Thanks!

@joshpar joshpar merged commit 4950bbb into supercollider:develop Aug 28, 2019
@nhthn nhthn mentioned this pull request Aug 28, 2019
mossheim added a commit that referenced this pull request Aug 28, 2019
@dyfer dyfer mentioned this pull request Dec 15, 2019
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
comp: class library SC class library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Plotter: ERROR: "Message '@' not understood"
3 participants