Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Incorrect (negative) values in safeRound result #134

Closed
wiese opened this issue Feb 23, 2023 · 5 comments
Closed

Incorrect (negative) values in safeRound result #134

wiese opened this issue Feb 23, 2023 · 5 comments

Comments

@wiese
Copy link

wiese commented Feb 23, 2023

For certain inputs (not entirely sure what they have in common) we are seeing results which do seem off and do not match Iteround's results for the same input.

Interestingly, a correct sum seems to be achieved by negative and positive values compensating for one another.

Please find a reproduction at https://github.com/wiese/saferound/commit/0bacbbc99c3853f7d28711e02b6f108c4c298092

The iteround equivalent (codesandbox here):

import iteround

data = {
    'a': 0.013245557004700679,
    'b': 0.7268604797100215,
    'c': 0.6155001637295456,
    'd': 0.06875329582046738,
    'e': 0.8387831576913838,
    'f': 0.03243429881206358,
    'g': 0.0023860988000495792,
    'h': 0.41960946536150784,
    'i': 0.675551119386058,
    'j': 0.6409523907667709,
    'k': 0.9659239729174308,
}

print(sum(data.values())) // 4.999999999999999

rounded = iteround.saferound(data, 0)

print(sum(rounded.values())) // 5.0
print(rounded.values()) // dict_values([0.0, 1.0, 0.0, 0.0, 1.0, 0.0, 0.0, 0.0, 1.0, 1.0, 1.0])

Please advise, am I missing something? Thank you!

boredland added a commit that referenced this issue Feb 24, 2023
when porting iteround, the different sorting methods were skipped
in favor of diffing-only. apparently I overshot the mark a bit in
deleting supposedly unused code paths. This should bring the
implementation a good deal closer to the original and compensate
for corresponding deviations.

contributes to #134
@boredland
Copy link
Owner

Well, I guess I removed too much comparing with the original implementation. Pushed a bug fix that should align it. At least your test became green and while the order of things changed, the sum-result remained stable throughout the pre-exising tests.

@wiese
Copy link
Author

wiese commented Feb 27, 2023

Thanks for the swift reaction. The suggested regression test is now green.

It was my understanding that saferound tries to produce the same results as iteround.

However, after this change that's not the case for the pre-existing example in the test suite

input 4.0001, 3.2345, 3.2321, 6.4523, 5.3453, 7.3422
saferound 5, 3, 4, 6, 5, 7
iteround 4.0, 3.0, 3.0, 7.0, 6.0, 7.0

nor for the example of the iteround package

input 60.19012964572332, 15.428802458406679, 24.381067895870007
saferound 61, 15, 24
iteround 60.0, 16.0, 24.0

Is there a misunderstanding?

Thanks!

@boredland
Copy link
Owner

boredland commented Feb 27, 2023

Yes, it was meant to be copy of the "DIFFERENCE" strategy of iteround. Perhaps I have overseen a sorting or something. Feel free to raise a PR ;-)

@boredland
Copy link
Owner

nvm, I think I have been able to fix it so, that the original basic difference test as well as the readme example are green.

boredland added a commit that referenced this issue Feb 27, 2023
the original library contains a "reverse" mechanism, that influences
which element is being rounded next. added tests proving that it aligns
with the readme example and the examples from the "basic_difference" test.

contributes to #134
@wiese
Copy link
Author

wiese commented Mar 21, 2023

Thanks for your work!

That last change addressed our concerns.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants