Issue 397551005: tryton-env: Accrue and allocate rounding errors when rounding taxes
DescriptionPatch Set 1 #Patch Set 2 : Round on line taxes only when rounding per line + better residual rounding #
Total comments: 18
Patch Set 3 : Use copy_sign + split tests + reverse on round() #Patch Set 4 : Update to tip #Patch Set 5 : Update to tip #Patch Set 6 : Update to tip #
MessagesTotal messages: 19
checks OK URL: https://codereview.tryton.org/397551005
Sign in to reply to this message.
ERROR: /tmp/reviewbot-u4zcidf3/modules/account/tests/test_account.py Imports are incorrectly sorted and/or formatted. URL: https://codereview.tryton.org/397551005
Sign in to reply to this message.
https://codereview.tryton.org/397551005/diff/397651003/modules/account/CHANGELOG File modules/account/CHANGELOG (right): https://codereview.tryton.org/397551005/diff/397651003/modules/account/CHANGE... modules/account/CHANGELOG:1: * Accrue and allocate rounding errors when there is multiple taxes there are... https://codereview.tryton.org/397551005/diff/397651003/modules/account/tax.py File modules/account/tax.py (right): https://codereview.tryton.org/397551005/diff/397651003/modules/account/tax.py... modules/account/tax.py:1198: }[self.currency.round.__defaults__[0]] Maybe we could force to use ROUND_HALF_EVEN as the rounding method here should not have impact on the final amount. https://codereview.tryton.org/397551005/diff/397651003/modules/account/tax.py... modules/account/tax.py:1228: taxes_to_round = [] maybe you could store in a dict so you do not need to loop over to create a new one. https://codereview.tryton.org/397551005/diff/397651003/modules/account/tests/... File modules/account/tests/test_account.py (right): https://codereview.tryton.org/397551005/diff/397651003/modules/account/tests/... modules/account/tests/test_account.py:1123: "Test TaxableMixin with rounding with residual amount" Maybe: Test TaxableMixin for rounding residual amount https://codereview.tryton.org/397551005/diff/397651003/modules/account/tests/... modules/account/tests/test_account.py:1172: taxable = self.Taxable( I think it will be better to have a second test.
Sign in to reply to this message.
https://codereview.tryton.org/397551005/diff/397651003/modules/account/tax.py File modules/account/tax.py (right): https://codereview.tryton.org/397551005/diff/397651003/modules/account/tax.py... modules/account/tax.py:1198: }[self.currency.round.__defaults__[0]] On 2022/03/25 14:49:35, ced wrote: > Maybe we could force to use ROUND_HALF_EVEN as the rounding method here should > not have impact on the final amount. In fact it was my original code and indeed it does not change the outcome (I tested creating many taxes and so on). But using the inverse (which result in the same computation) seems more logical. https://codereview.tryton.org/397551005/diff/397651003/modules/account/tests/... File modules/account/tests/test_account.py (right): https://codereview.tryton.org/397551005/diff/397651003/modules/account/tests/... modules/account/tests/test_account.py:1172: taxable = self.Taxable( On 2022/03/25 14:49:35, ced wrote: > I think it will be better to have a second test. Testing what?
Sign in to reply to this message.
https://codereview.tryton.org/397551005/diff/397651003/modules/account/tax.py File modules/account/tax.py (right): https://codereview.tryton.org/397551005/diff/397651003/modules/account/tax.py... modules/account/tax.py:1198: }[self.currency.round.__defaults__[0]] On 2022/03/25 18:53:28, nicoe wrote: > On 2022/03/25 14:49:35, ced wrote: > > Maybe we could force to use ROUND_HALF_EVEN as the rounding method here should > > not have impact on the final amount. > > In fact it was my original code and indeed it does not change the outcome (I > tested creating many taxes and so on). > > But using the inverse (which result in the same computation) seems more logical. I do not understand the rational. If you always round ROUND_HALF_EVEN, it should have the same behavior. https://codereview.tryton.org/397551005/diff/397651003/modules/account/tests/... File modules/account/tests/test_account.py (right): https://codereview.tryton.org/397551005/diff/397651003/modules/account/tests/... modules/account/tests/test_account.py:1172: taxable = self.Taxable( On 2022/03/25 18:53:28, nicoe wrote: > On 2022/03/25 14:49:35, ced wrote: > > I think it will be better to have a second test. > > Testing what? The same thing but separately for each case.
Sign in to reply to this message.
https://codereview.tryton.org/397551005/diff/397651003/modules/account/tax.py File modules/account/tax.py (right): https://codereview.tryton.org/397551005/diff/397651003/modules/account/tax.py... modules/account/tax.py:1198: }[self.currency.round.__defaults__[0]] On 2022/03/25 20:21:56, ced wrote: > On 2022/03/25 18:53:28, nicoe wrote: > > On 2022/03/25 14:49:35, ced wrote: > > > Maybe we could force to use ROUND_HALF_EVEN as the rounding method here > should > > > not have impact on the final amount. > > > > In fact it was my original code and indeed it does not change the outcome (I > > tested creating many taxes and so on). > > > > But using the inverse (which result in the same computation) seems more > logical. > > I do not understand the rational. If you always round ROUND_HALF_EVEN, it should > have the same behavior. If you do ROUND_UP then ROUND_HALF_EVEN will sometimes go towards zero and sometimes towards Infinity, thus not counterbalancing the effect of the rounding. That's the reasoning. But I tried to display this behaviour (by having a lot of taxes) and it didn't work. https://codereview.tryton.org/397551005/diff/397651003/modules/account/tests/... File modules/account/tests/test_account.py (right): https://codereview.tryton.org/397551005/diff/397651003/modules/account/tests/... modules/account/tests/test_account.py:1172: taxable = self.Taxable( On 2022/03/25 20:21:56, ced wrote: > On 2022/03/25 18:53:28, nicoe wrote: > > On 2022/03/25 14:49:35, ced wrote: > > > I think it will be better to have a second test. > > > > Testing what? > > The same thing but separately for each case. OK you mean the test should be splitted … I agree.
Sign in to reply to this message.
https://codereview.tryton.org/397551005/diff/397651003/modules/account/tax.py File modules/account/tax.py (right): https://codereview.tryton.org/397551005/diff/397651003/modules/account/tax.py... modules/account/tax.py:1198: }[self.currency.round.__defaults__[0]] On 2022/03/26 08:27:01, nicoe wrote: > On 2022/03/25 20:21:56, ced wrote: > > On 2022/03/25 18:53:28, nicoe wrote: > > > On 2022/03/25 14:49:35, ced wrote: > > > > Maybe we could force to use ROUND_HALF_EVEN as the rounding method here > > should > > > > not have impact on the final amount. > > > > > > In fact it was my original code and indeed it does not change the outcome (I > > > tested creating many taxes and so on). > > > > > > But using the inverse (which result in the same computation) seems more > > logical. > > > > I do not understand the rational. If you always round ROUND_HALF_EVEN, it > should > > have the same behavior. > > If you do ROUND_UP then ROUND_HALF_EVEN will sometimes go towards zero and > sometimes towards Infinity, thus not counterbalancing the effect of the > rounding. That's the reasoning. But I mean to always do ROUND_HALF_EVEN.
Sign in to reply to this message.
https://codereview.tryton.org/397551005/diff/397651003/modules/account/tax.py File modules/account/tax.py (right): https://codereview.tryton.org/397551005/diff/397651003/modules/account/tax.py... modules/account/tax.py:1198: }[self.currency.round.__defaults__[0]] On 2022/03/26 09:50:36, ced wrote: > On 2022/03/26 08:27:01, nicoe wrote: > > On 2022/03/25 20:21:56, ced wrote: > > > On 2022/03/25 18:53:28, nicoe wrote: > > > > On 2022/03/25 14:49:35, ced wrote: > > > > > Maybe we could force to use ROUND_HALF_EVEN as the rounding method here > > > should > > > > > not have impact on the final amount. > > > > > > > > In fact it was my original code and indeed it does not change the outcome > (I > > > > tested creating many taxes and so on). > > > > > > > > But using the inverse (which result in the same computation) seems more > > > logical. > > > > > > I do not understand the rational. If you always round ROUND_HALF_EVEN, it > > should > > > have the same behavior. > > > > If you do ROUND_UP then ROUND_HALF_EVEN will sometimes go towards zero and > > sometimes towards Infinity, thus not counterbalancing the effect of the > > rounding. That's the reasoning. > > But I mean to always do ROUND_HALF_EVEN. > Indeed I think the best design would be to add a reverse flag on Currency.round to invert the rounding. It would be cleaner than doing introspection of default value because it could be changed by code and also it may be useful for other similar use case.
Sign in to reply to this message.
https://codereview.tryton.org/397551005/diff/397651003/modules/account/tax.py File modules/account/tax.py (right): https://codereview.tryton.org/397551005/diff/397651003/modules/account/tax.py... modules/account/tax.py:1198: }[self.currency.round.__defaults__[0]] On 2022/03/26 09:50:36, ced wrote: > On 2022/03/26 08:27:01, nicoe wrote: > > On 2022/03/25 20:21:56, ced wrote: > > > On 2022/03/25 18:53:28, nicoe wrote: > > > > On 2022/03/25 14:49:35, ced wrote: > > > > > Maybe we could force to use ROUND_HALF_EVEN as the rounding method here > > > should > > > > > not have impact on the final amount. > > > > > > > > In fact it was my original code and indeed it does not change the outcome > (I > > > > tested creating many taxes and so on). > > > > > > > > But using the inverse (which result in the same computation) seems more > > > logical. > > > > > > I do not understand the rational. If you always round ROUND_HALF_EVEN, it > > should > > > have the same behavior. > > > > If you do ROUND_UP then ROUND_HALF_EVEN will sometimes go towards zero and > > sometimes towards Infinity, thus not counterbalancing the effect of the > > rounding. That's the reasoning. > > But I mean to always do ROUND_HALF_EVEN. > Yes I understood. I just explained the reasoning. https://codereview.tryton.org/397551005/diff/397651003/modules/account/tax.py... modules/account/tax.py:1198: }[self.currency.round.__defaults__[0]] On 2022/03/26 10:47:50, ced wrote: > On 2022/03/26 09:50:36, ced wrote: > > On 2022/03/26 08:27:01, nicoe wrote: > > > On 2022/03/25 20:21:56, ced wrote: > > > > On 2022/03/25 18:53:28, nicoe wrote: > > > > > On 2022/03/25 14:49:35, ced wrote: > > > > > > Maybe we could force to use ROUND_HALF_EVEN as the rounding method > here > > > > should > > > > > > not have impact on the final amount. > > > > > > > > > > In fact it was my original code and indeed it does not change the > outcome > > (I > > > > > tested creating many taxes and so on). > > > > > > > > > > But using the inverse (which result in the same computation) seems more > > > > logical. > > > > > > > > I do not understand the rational. If you always round ROUND_HALF_EVEN, it > > > should > > > > have the same behavior. > > > > > > If you do ROUND_UP then ROUND_HALF_EVEN will sometimes go towards zero and > > > sometimes towards Infinity, thus not counterbalancing the effect of the > > > rounding. That's the reasoning. > > > > But I mean to always do ROUND_HALF_EVEN. > > > > Indeed I think the best design would be to add a reverse flag on Currency.round > to invert the rounding. It would be cleaner than doing introspection of default > value because it could be changed by code and also it may be useful for other > similar use case. Acknowledged.
Sign in to reply to this message.
https://codereview.tryton.org/397551005/diff/397651003/modules/account/CHANGELOG File modules/account/CHANGELOG (right): https://codereview.tryton.org/397551005/diff/397651003/modules/account/CHANGE... modules/account/CHANGELOG:1: * Accrue and allocate rounding errors when there is multiple taxes On 2022/03/25 14:49:35, ced wrote: > there are... Done. https://codereview.tryton.org/397551005/diff/397651003/modules/account/tax.py File modules/account/tax.py (right): https://codereview.tryton.org/397551005/diff/397651003/modules/account/tax.py... modules/account/tax.py:1228: taxes_to_round = [] On 2022/03/25 14:49:35, ced wrote: > maybe you could store in a dict so you do not need to loop over to create a new > one. Done. https://codereview.tryton.org/397551005/diff/397651003/modules/account/tests/... File modules/account/tests/test_account.py (right): https://codereview.tryton.org/397551005/diff/397651003/modules/account/tests/... modules/account/tests/test_account.py:1123: "Test TaxableMixin with rounding with residual amount" On 2022/03/25 14:49:35, ced wrote: > Maybe: Test TaxableMixin for rounding residual amount Done.
Sign in to reply to this message.
checks OK URL: https://codereview.tryton.org/397551005
Sign in to reply to this message.
checks OK URL: https://codereview.tryton.org/397551005
Sign in to reply to this message.
checks OK URL: https://codereview.tryton.org/397551005
Sign in to reply to this message.
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
