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

Add options to enforce min/max exclusion #73

Closed
mbarlow12 opened this issue May 24, 2018 · 12 comments
Closed

Add options to enforce min/max exclusion #73

mbarlow12 opened this issue May 24, 2018 · 12 comments
Assignees

Comments

@mbarlow12
Copy link
Contributor

The range type doesn't currently support arbitrarily set closed/open/half-open intervals. This feature may be useful in various settings (it arose when mapping alert text to non-zero input values in phetsims/molecules-and-light#180).

I'm mildly concerned about how JS handles certain floating point situations (e.g. var x = 0.1 * 0.2 = 0.020000000000000004) and how they would interact with an exclusive Range( 0.02, 0.03 ). In that case, x should not be included in the Range.

Proposal is to implement closed intervals by default, add option keys (excludeMin and excludeMax) for closed behavior, and create tests to ensure these additions don't break current usages.

@mbarlow12 mbarlow12 self-assigned this May 24, 2018
@jonathanolson
Copy link
Contributor

As noted in Slack, these changes sound good. I don't think Range is used in performance-critical areas, so adding the options object should be totally fine (and helps for code readability).

I'm mildly concerned about how JS handles certain floating point situations

Yup, seems somewhat unavoidable. Generally the client should round values as necessary, and it's best to NOT round in range checks. Can always use Fraction if we need to handle those fractional cases with full accuracy.

@mbarlow12
Copy link
Contributor Author

Thanks for the best practices info @jonathanolson, I'll be sure to add that in the doc.

@mbarlow12
Copy link
Contributor Author

After some review and discussion with @zepumph, I opted to create a new type OpenRange that overrides the appropriate functions (contains, containsRange, intersects) to allow for both fully open and half open ranges.

The primary question left is how to handle constrainValue. The best option I can think of would be to call Range.constrainValue then increase/decrease the value by an equal precision to the passed-in value. That seems expensive, and would require the use of Util.numberOfDecimalPlaces. @jonathanolson do you have any input on this?

@mbarlow12
Copy link
Contributor Author

Unit tests added as well in the above commit.

mbarlow12 added a commit that referenced this issue Aug 6, 2018
@zepumph
Copy link
Member

zepumph commented Aug 14, 2018

This looks really nice thanks for doing that!

@mbarlow12
Copy link
Contributor Author

Thanks @zepumph! I think I can close this.

@pixelzoom
Copy link
Contributor

pixelzoom commented Jan 7, 2019

Reopening.

The documentation header on OpenRangeTests looks like a copy-paste error. Should be "OpenRange tests" with @author Michael Barlow:

/**
 * Bounds2 tests
 *
 * @author Jonathan Olson (PhET Interactive Simulations)
 * @author Sam Reid (PhET Interactive Simulations)
 */

And there's dead code at the bottom of the file:

  // QUnit.test( 'assertion failures', function( assert ) {
  //   debugger;
  //   assert.throws( new OpenRange( 1, 1, minHalfOpenOptions ), 'min open range with min === max throws an error' );
  //   assert.throws( new OpenRange( 1, 1, maxHalfOpenOptions ), 'max open range with min === max throws an error' );
  //   assert.throws( new OpenRange( 1, 1 ), 'full open range with min === max throws an error' );
  // } );

@mbarlow12
Copy link
Contributor Author

Thanks @pixelzoom. Code uncommented, annotations updated, and tests passing. Assigning to you to close.

@mbarlow12 mbarlow12 assigned pixelzoom and unassigned mbarlow12 Jan 7, 2019
@pixelzoom
Copy link
Contributor

👍 Closing.

@pixelzoom
Copy link
Contributor

Reopening. OpenRangeTests is failing CT:

dot : top-level-unit-tests : require.js?brand=phet-io
78 out of 80 tests passed. 2 failed.
OpenRange: setter overrides failed:
cannot set min = max in OpenRange
    at Object.<anonymous> (https://bayes.colorado.edu/continuous-testing/snapshot-1546990372093/dot/js/OpenRangeTests.js?bust=1546993710877:60:12)

OpenRange: setter overrides failed:
cannot set min = max in OpenRange
    at Object.<anonymous> (https://bayes.colorado.edu/continuous-testing/snapshot-1546990372093/dot/js/OpenRangeTests.js?bust=1546993710877:63:12)

OpenRange: assertion failures failed:
include both min and max throws an error
    at Object.<anonymous> (https://bayes.colorado.edu/continuous-testing/snapshot-1546990372093/dot/js/OpenRangeTests.js?bust=1546993710877:67:12)

OpenRange: assertion failures failed:
min open range with min === max throws an error
    at Object.<anonymous> (https://bayes.colorado.edu/continuous-testing/snapshot-1546990372093/dot/js/OpenRangeTests.js?bust=1546993710877:68:12)

OpenRange: assertion failures failed:
max open range with min === max throws an error
    at Object.<anonymous> (https://bayes.colorado.edu/continuous-testing/snapshot-1546990372093/dot/js/OpenRangeTests.js?bust=1546993710877:69:12)

OpenRange: assertion failures failed:
full open range with min === max throws an error
    at Object.<anonymous> (https://bayes.colorado.edu/continuous-testing/snapshot-1546990372093/dot/js/OpenRangeTests.js?bust=1546993710877:70:12)

OpenRange: assertion failures failed:
setting min equal to max throws an error
    at Object.<anonymous> (https://bayes.colorado.edu/continuous-testing/snapshot-1546990372093/dot/js/OpenRangeTests.js?bust=1546993710877:73:12)

OpenRange: assertion failures failed:
setting min greater than max throws an error
    at Object.<anonymous> (https://bayes.colorado.edu/continuous-testing/snapshot-1546990372093/dot/js/OpenRangeTests.js?bust=1546993710877:75:12)

OpenRange: assertion failures failed:
setting max equal to min throws an error
    at Object.<anonymous> (https://bayes.colorado.edu/continuous-testing/snapshot-1546990372093/dot/js/OpenRangeTests.js?bust=1546993710877:77:12)

OpenRange: assertion failures failed:
setting max less than min throws an error
    at Object.<anonymous> (https://bayes.colorado.edu/continuous-testing/snapshot-1546990372093/dot/js/OpenRangeTests.js?bust=1546993710877:79:12)


id: Bayes Chrome
Approximately 1/8/2019, 4:32:52 PM
dot : top-level-unit-tests : require.js?ea
80 out of 80 tests passed. 0 failed.

Approximately 1/8/2019, 4:32:52 PM

@mbarlow12
Copy link
Contributor Author

@zepumph helped in making a check that window.assert is active whenever we check for assertion errors with assert.throws. Should be fixed.

@pixelzoom
Copy link
Contributor

All dot tests are passing with and without ?ea in phetmarks. Closing.

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

No branches or pull requests

4 participants