Issue 397551005: tryton-env: Accrue and allocate rounding errors when rounding taxes (Closed)
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 #Patch Set 7 : Update to tip #
Total comments: 10
Patch Set 8 : Change names and add test for opposite rounding #
Total comments: 4
MessagesTotal messages: 27
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.
checks OK URL: https://codereview.tryton.org/397551005
Sign in to reply to this message.
It will be good to have tests about opposite rounding. https://codereview.tryton.org/397551005/diff/423431009/modules/account/tax.py File modules/account/tax.py (right): https://codereview.tryton.org/397551005/diff/423431009/modules/account/tax.py... modules/account/tax.py:1179: residual_amount = 0 is it not simpler to name it remainder https://codereview.tryton.org/397551005/diff/423431009/modules/account/tax.py... modules/account/tax.py:1212: line_taxes = {} I'm wondering if it will not be better named: current_taxes https://codereview.tryton.org/397551005/diff/423431009/modules/account/tests/... File modules/account/tests/test_module.py (right): https://codereview.tryton.org/397551005/diff/423431009/modules/account/tests/... modules/account/tests/test_module.py:1286: tax1.credit_note_account = tax_account for me there should be at least 2 taxes otherwise there is no residual. https://codereview.tryton.org/397551005/diff/423431009/modules/currency/CHANG... File modules/currency/CHANGELOG (right): https://codereview.tryton.org/397551005/diff/423431009/modules/currency/CHANG... modules/currency/CHANGELOG:1: * Add reverse on .round() I think it is better: Allow rounding amount with opposite rounding method https://codereview.tryton.org/397551005/diff/423431009/modules/currency/curre... File modules/currency/currency.py (right): https://codereview.tryton.org/397551005/diff/423431009/modules/currency/curre... modules/currency/currency.py:167: def round(self, amount, rounding=decimal.ROUND_HALF_EVEN, reverse=False): is it not better named 'opposite' like in opposite direction because reverse seems to imply that it will do the inverse of rounding.
Sign in to reply to this message.
https://codereview.tryton.org/397551005/diff/423431009/modules/account/tax.py File modules/account/tax.py (right): https://codereview.tryton.org/397551005/diff/423431009/modules/account/tax.py... modules/account/tax.py:1179: residual_amount = 0 On 2022/08/31 11:53:05, ced wrote: > is it not simpler to name it remainder Done. https://codereview.tryton.org/397551005/diff/423431009/modules/account/tax.py... modules/account/tax.py:1212: line_taxes = {} On 2022/08/31 11:53:05, ced wrote: > I'm wondering if it will not be better named: current_taxes Done. https://codereview.tryton.org/397551005/diff/423431009/modules/account/tests/... File modules/account/tests/test_module.py (right): https://codereview.tryton.org/397551005/diff/423431009/modules/account/tests/... modules/account/tests/test_module.py:1286: tax1.credit_note_account = tax_account On 2022/08/31 11:53:05, ced wrote: > for me there should be at least 2 taxes otherwise there is no residual. Done. https://codereview.tryton.org/397551005/diff/423431009/modules/currency/CHANG... File modules/currency/CHANGELOG (right): https://codereview.tryton.org/397551005/diff/423431009/modules/currency/CHANG... modules/currency/CHANGELOG:1: * Add reverse on .round() On 2022/08/31 11:53:05, ced wrote: > I think it is better: > > Allow rounding amount with opposite rounding method Done. https://codereview.tryton.org/397551005/diff/423431009/modules/currency/curre... File modules/currency/currency.py (right): https://codereview.tryton.org/397551005/diff/423431009/modules/currency/curre... modules/currency/currency.py:167: def round(self, amount, rounding=decimal.ROUND_HALF_EVEN, reverse=False): On 2022/08/31 11:53:05, ced wrote: > is it not better named 'opposite' like in opposite direction because reverse > seems to imply that it will do the inverse of rounding. Done.
Sign in to reply to this message.
https://codereview.tryton.org/397551005/diff/424061003/modules/currency/tests... modules/currency/tests/test_module.py:166: too many blank lines (2) URL: https://codereview.tryton.org/397551005
Sign in to reply to this message.
https://codereview.tryton.org/397551005/diff/424061003/modules/currency/curre... File modules/currency/currency.py (right): https://codereview.tryton.org/397551005/diff/424061003/modules/currency/curre... modules/currency/currency.py:171: rounding = ROUNDING_REVERSES[rounding] For consistency it should be opposite or reverse. https://codereview.tryton.org/397551005/diff/424061003/modules/currency/tests... File modules/currency/tests/test_module.py (right): https://codereview.tryton.org/397551005/diff/424061003/modules/currency/tests... modules/currency/tests/test_module.py:154: self.assertEqual(rounded, Decimal('1.24')) I do not see the point of this test. It is better to follow AAA https://codereview.tryton.org/397551005/diff/424061003/modules/currency/tests... modules/currency/tests/test_module.py:159: Currency.round, '__defaults__', (ROUND_HALF_DOWN, False)): I do not see the point to hack defaults, just pass ROUND_HALF_DOWN to the call. https://codereview.tryton.org/397551005/diff/424061003/modules/currency/tests... modules/currency/tests/test_module.py:163: self.assertEqual(opposite_rounded, Decimal('1.24')) This should be a second test.
Sign in to reply to this message.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
