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.

Sign up for GitHub

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

Jump to bottom

sclang: fix signal_thresh_xf typo #5432

Merged
merged 1 commit into from
Oct 25, 2022
Merged

sclang: fix signal_thresh_xf typo #5432

merged 1 commit into from
Oct 25, 2022

Conversation

elgiano
Copy link
Contributor

@elgiano elgiano commented Apr 25, 2021 •
edited
Loading

Purpose and Motivation

When calling .thresh on a Signal, the supplied threshold is squared:

Signal.sineFill(1024, [1]).thresh(0.5).reject(_==0).minItem
// -> ~0.25
// Signal.thresh zeroes out everything below threshold.squared?

// as expected SimpleNumber.thresh zeroes out everything below a threshold
({1.0.rand2}!1024).thresh(0.5).reject(_==0).minItem
// -> ~0.5

I would expect thresh to do the same operation on SimpleNumbers and Signals. Am I missing something?
The threshold gets squared only for signal_thresh_fx, in lang/LangSource/PyrSignal.cpp:

supercollider/lang/LangSource/PyrSignal.cpp

Line 216 in 6de068b

float inb2 = inb * inb;

It looks like a copy-paste leftover from signal_ring4_xf, which is just above. Removing this line produces the expected behavior.

Types of changes

  • Bug fix

To-do list

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

@elgiano elgiano added bug Issues that relate to unexpected/unwanted behavior. Don't use for PRs. comp: sclang sclang C++ implementation (primitives, etc.). for changes to class lib use "comp: class library" labels Apr 25, 2021
@elgiano elgiano force-pushed the topic/signalThresh branch from dd64986 to ae96fcd Compare April 25, 2021 12:00
joshpar
joshpar requested changes Feb 20, 2022
View reviewed changes
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.

@elgiano - How do you think we should document this change? If any, can you please include it in the PR and tag me? If no documentation changes are needed, I'll approve

@elgiano
Copy link
Contributor Author

elgiano commented Feb 20, 2022

I think we could put a note that the function had a bug until the version where we fix it, like https://doc.sccode.org/Classes/Buffer.html#-cheby

What do you think about something like this? @joshpar

note::
In older versions, this function was implemented incorrectly, evaluating the square of provided threshold. This behavior is fixed starting from SuperCollider 3.13. However, note that older code might now give different results.
::

@dyfer
However, I'm not sure why is this labeled as API change. Doesn't "API change" only refer to changes to files in include?

@jamshark70
Copy link
Contributor

I'm not sure why is this labeled as API change. Doesn't "API change" only refer to changes to files in include?

No. If there's version A and version B, and you run the same sclang code in both versions but the result is different (or the same server messages), that's an API change.

@dyfer
Copy link
Member

dyfer commented Oct 24, 2022

@elgiano could you add the note in the documentation? I have a suggestion to put the version at the beginning of the sentence, what do you think?

note::
Before SuperCollider 3.13 this function was implemented incorrectly, evaluating the square of provided threshold. This behavior is now fixed, but note that older code might give different results.
::

@elgiano elgiano force-pushed the topic/signalThresh branch from ae96fcd to bdb93ad Compare October 24, 2022 22:26
@elgiano
Copy link
Contributor Author

elgiano commented Oct 24, 2022

Sure, but we miss the whole method, so I put this:

method::thresh
Thresholding replaces every value < threshold with 0.
note::
Before SuperCollider 3.13 this function was implemented incorrectly, evaluating the square of provided threshold. This behavior is now fixed, but note that older code might give different results.
::

I've also put it to the top of the list of Binary Messages is it would be the only one with a description.

@elgiano elgiano force-pushed the topic/signalThresh branch from bdb93ad to 86ca28b Compare October 24, 2022 22:27
@dyfer
Copy link
Member

dyfer commented Oct 25, 2022

Thanks @elgiano !

@dyfer dyfer merged commit 6d06f07 into supercollider:develop Oct 25, 2022
@elgiano elgiano deleted the topic/signalThresh branch October 26, 2022 13:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joshpar joshpar joshpar approved these changes

Assignees
No one assigned
Labels
API change bug Issues that relate to unexpected/unwanted behavior. Don't use for PRs. comp: sclang sclang C++ implementation (primitives, etc.). for changes to class lib use "comp: class library"
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@elgiano @jamshark70 @dyfer @joshpar