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

Unexpected behavior for step rounding according to spec in Integer/Float ranges when min or max are missing #301

Open
schaze opened this issue Oct 7, 2024 · 13 comments

Comments

@schaze
Copy link
Contributor

schaze commented Oct 7, 2024

Description:

The current spec for handling integer/float ranges with steps, [min]:[max][:step], specifies that the base for calculating a proper value based on the step should be min, max, or the current property value (in that order). However, the behavior when either min or max is missing results in different rounding behavior that is not explicitly clarified in the spec.

Specifically, the behavior of rounding changes depending on whether the base for step rounding is derived from the min or max value. This leads to different rounding results in cases such as 0:10:2 vs :10:2, where:

  • In the case of 0:10:2, the base is min (0), and the rounding behaves as expected, rounding values upwards from 0.
  • In the case of :10:2 (no lower bound), the base is max (10), which results in rounding values downwards from 10.

This results in inconsistencies that may not be immediately obvious from reading the spec.

Examples:

  1. Example 1: Range 0:10:2

    • This range specifies a minimum of 0, a maximum of 10, and a step of 2. The base for rounding is min (0), so rounding occurs upwards from the base.
    • Rounding behavior:
    Value Rounded Value Result
    0 0 Success
    1 2 Success
    2 2 Success
    3 4 Success
    4 4 Success
    5 6 Success
    6 6 Success
    7 8 Success
    8 8 Success
    9 10 Success
    10 10 Success
    11 12 - Out of range Error
  2. Example 2: Range :10:2

    • This range specifies no lower bound, a maximum of 10, and a step of 2. The base for rounding is max (10), which causes rounding to occur downwards from the maximum value.
    • Rounding behavior:
    Value Rounded Value Result
    0 0 Success (rounded down from max)
    1 0 Success (rounded down from max)
    2 2 Success
    3 2 Success
    4 4 Success
    5 4 Success
    6 6 Success
    7 6 Success
    8 8 Success
    9 8 Success (rounded down from max)
    10 10 Success
    11 12 - Out of range Error

Issue:

The spec does not clearly indicate the expected behavior when rounding from max vs. min. While the spec indicates that the base for step calculation should be either min, max, or the current property value (in that order), it is not clear that the rounding behavior would differ significantly depending on which base is used. This could lead to confusion for implementers who expect consistent rounding behavior regardless of whether min or max is specified.

Request:

We should improve the spec by:

  1. Explicitly stating how rounding behavior changes based on whether min or max is used as the base for step calculation.
  2. Providing examples to demonstrate how values are rounded when min is missing (i.e., rounding down from max).

This clarification would help ensure consistent understanding of how the spec should be implemented and what behavior is expected.

Or think about if we want to adjust the spec to ensure the rounding results stay consistent in both cases.

@schaze schaze assigned Tieske and schaze and unassigned Tieske and schaze Oct 7, 2024
@Tieske
Copy link
Contributor

Tieske commented Oct 8, 2024

Nice catch.

The 2nd example is mathematically correct, but counter intuitive imho. So we should specify how it should round in order to result in the intuitive value.

@Tieske
Copy link
Contributor

Tieske commented Oct 8, 2024

base should be calculated like this I think:

base = 0
if mimimum != null then
  base = minimum
elif maximum != null then
  base = maximum
else
  base = current_value
end

// reduce base to below input value, to have min+max behave the same
if base > input_value then
  base = round((input_value - base) / step - 1, 0) * step + base
end

@Tieske
Copy link
Contributor

Tieske commented Oct 8, 2024

Wondering; should we be creating a generic test suite for all sorts of validations? That would ease writing homie libraries quite a bit I think

  • list of valid IDs
  • list of invalid IDs
  • per type
    • per formats
      • list of valid formats
      • list of invalid formats
      • list of corrected values (eg. rounding etc)

similar to these json schema ones: https://github.com/json-schema-org/JSON-Schema-Test-Suite/tree/main/tests/draft4

@schaze
Copy link
Contributor Author

schaze commented Oct 8, 2024

I actually stumbled across this while writing extensive tests for my rust library.
It provides a fully compliant parser for the formats and value types as well as validation of data input according to the property description. Maybe we could use this already, but I am not sure what exactly you have in mind (JSON Schema validation will most likely not be able to cover all our formatting rules...).

@Tieske
Copy link
Contributor

Tieske commented Oct 9, 2024

sorry should have been more clear. Intent is not to use JSON schema for validation, but writing test cases in JSON, like this one: https://github.com/json-schema-org/JSON-Schema-Test-Suite/blob/main/tests/draft4/anyOf.json it is a file that lists all the test cases, and expected results for the JSONschema "anyOf" operator.

So instead of defining all the testcases yourself, you parse the JSON file, and in a loop run all the tests contained within it.

Example: This module; https://github.com/Tieske/lua-resty-ljsonschema/tree/master/spec contains the repo above as a submodule. In the test suite tyhe files are loaded, and tests executed: https://github.com/Tieske/lua-resty-ljsonschema/blob/master/spec/suite_spec.lua

@Tieske
Copy link
Contributor

Tieske commented Oct 9, 2024

we could create one centralized set of tests, here in the homiet github org, and every library can run tests against those sets. That would cover your comment above:

This could lead to confusion for implementers who expect consistent rounding behavior regardless of whether min or max is specified.

@schaze
Copy link
Contributor Author

schaze commented Oct 10, 2024

Ah now I get it. Yes I think that is a great idea!

@Tieske
Copy link
Contributor

Tieske commented Oct 10, 2024

first attempt here: https://github.com/homieiot/homie-testsuite/blob/main/homie5/formats/boolean.yml

please check, then we can add the numeric stuff as well

PS. I used yaml instead of json, because it supports comments

@schaze
Copy link
Contributor Author

schaze commented Oct 12, 2024

I would prefer having the defining properties of the test under one attribute so we can directly parse it in as the object to be tested where applicable:

tests:
  - description: proper format is accepted
    property:
      type: boolean
      format: 'off,on'
    valid: true

This way I could parse the property description directly into the type used in my library also using the same mechanism as I do for the real thing instead of manually constructing a property description.

To make it more generic we could change it to:

tests:
  - description: proper format is accepted
    test_definition:
      type: boolean
      format: 'off,on'
    input_data: {}
    valid: true

naming is as always hard, so test_definition and input _data we're just the first thing that came to mind.

@Tieske
Copy link
Contributor

Tieske commented Oct 12, 2024

that's the part I struggled with. Changing it like that is fine by me

@schaze
Copy link
Contributor Author

schaze commented Oct 12, 2024

Let me try actually implementing this for my library before we do more definitions to see if it works out as expected. I should have some time on Sunday night.

@schaze
Copy link
Contributor Author

schaze commented Oct 13, 2024

@Tieske : I created a pull request to your test repo. Please have a look and let me know what you think.
homieiot/homie-testsuite#1

@Tieske
Copy link
Contributor

Tieske commented Oct 18, 2024

base should be calculated like this I think:

base = 0
if mimimum != null then
  base = minimum
elif maximum != null then
  base = maximum
else
  base = current_value
end

// reduce base to below input value, to have min+max behave the same
if base > input_value then
  base = round((input_value - base) / step - 1, 0) * step + base
end

The main issue is that when calculating the steps before rounding, we take the difference and then divide by step-size. If max is only given then typically the "distance in steps" becomes a negative number (and rounding goes the other way).

To prevent that, the base should be "stepped down" to be smaller than the value being rounded.

To take the math out of the above, it could also be done like this, using a loop, does the same thing but probably easier to grasp;

base = 0
if mimimum != null then
  base = minimum
elif maximum != null then
  base = maximum
else
  base = current_value
end

// reduce base to below input value, to have min+max behave the same
while base > input_value do
  base = base - step
end

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

2 participants