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

Round scaled sensor values #17

Merged
merged 2 commits into from
Oct 19, 2022
Merged

Round scaled sensor values #17

merged 2 commits into from
Oct 19, 2022

Conversation

fisher-alice
Copy link

@fisher-alice fisher-alice commented Oct 19, 2022

Now that the code-dot-org/johnny-five is updated to the latest release from upstream, this PR re-implements rounding scaled sensor values proposed as Change #1 in this investigation.

When I first investigated this last customization, I ran an App Lab program to see if sensor values were already displayed as integer values using the updated johnny-five package, and they are. In addition to this observation, I also noted that in lib/sensor.js within eventProcessing, once the median is computed, it is rounded and assigned to roundMedian at line 122 but is not rounded in johnny-five-deprecated. Thus, I thought that this third customization was not necessary.

However, after running the tests in PlaygroundComponentsTest.js in @code-dot-org, I saw that the setScale tests failed which helped me understand that the scaling occurs after the value is reached from the eventProcessing function.

I confirmed that this rounding is necessary with the following program.
Screen Shot 2022-10-19 at 12 41 15 PM

Before the customization:

before-update-scaled.mp4

After the customization:

after-update-scaled.mp4

I looked into why the sensor values in the johnny-five-deprecated package are still displayed as integer values despite not having the median rounded. The comment above the function median explains that to reduce the noise in sensor readings, collected samples from the sensor are collected and then sorted. The value at the center is selected for the final value emitted and because there are many duplicate numbers, the result is almost always an integer even where there are an even number of values in the sample.
However, I am happy to see they added the Math.round just in case!

Screen Shot 2022-10-19 at 3 03 42 PM

I posted a PR to update the johnny-five version to 2.1.0-cdo.1. However, I will first merge this change, and then update the version of the package.

Links

jira ticket - Re-add rounded scaled sensor values

Testing

I ran the @code-dot-org unit tests in files that import johnny-five. In particular, PlaygroundComponents.js had a handful of unit tests that specifically tested the setScale function.

@fisher-alice fisher-alice marked this pull request as ready for review October 19, 2022 18:09
@fisher-alice fisher-alice requested review from a team and epeach October 19, 2022 18:09
@@ -206,7 +205,9 @@ class Sensor extends Withinable {
}

mapped = Fn.fmap(raw, this.range[0], this.range[1], state.scale[0], state.scale[1]);
constrain = Fn.constrain(mapped, state.scale[0], state.scale[1]);
// Code.org: round scaled sensor values

Choose a reason for hiding this comment

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

Just curious - do we have a standard way of tagging Code.org specific updates in this repo? (is it just adding a comment with // Code.org:?)

Copy link
Author

Choose a reason for hiding this comment

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

Looking at prior PRs that added Code.org customizations, @islemaster seemed to consistently use this convention, as seen here.

lib/sensor.js Outdated
constrain = Fn.constrain(mapped, state.scale[0], state.scale[1]);
// Code.org: round scaled sensor values
rounded = Math.round(mapped);
constrain = Fn.constrain(rounded, state.scale[0], state.scale[1]);

Choose a reason for hiding this comment

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

nit: round inline? Fn.constrain(Math.round(mapped), ...)

Copy link
Author

Choose a reason for hiding this comment

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

Thanks! Just updated.

@fisher-alice fisher-alice requested a review from a team October 19, 2022 22:22
Copy link

@bencodeorg bencodeorg left a comment

Choose a reason for hiding this comment

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

LGTM -- does this mean that all code-dot-org circuit playground tests now pass while using your updated johnny-five fork? Or is "all code-dot-org circuit playground tests" not such an easy distinction to make?

@fisher-alice
Copy link
Author

LGTM -- does this mean that all code-dot-org circuit playground tests now pass while using your updated johnny-five fork? Or is "all code-dot-org circuit playground tests" not such an easy distinction to make?

All code-dot-org circuit playground tests within code-dot-org/apps/test/unit/lib/kits/maker/boards/circuitPlayground now pass while using the updated johnny-five fork locally - there are 9 of them. Thanks!

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.

3 participants