-
Notifications
You must be signed in to change notification settings - Fork 3
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
transaction calculations: fixes #9 #20
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good! Logic makes sense but I would suggest just adjusting the tests slightly to make them easier to understand.
# the `value_local` dict has the total value of the transaction listed in each country's local currency; this | ||
# adjusts it according to proportion of total allocated to sector/country | ||
self.flat_transaction['value_local'] = dict([(country, (value_local * sector_pct_adjustment)) for | ||
country, value_local in self.transaction.value_local.value.items()]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see! Previously, the value_local
was being set and multiplied by a fraction (for the sector and country) each time so it became very small.
expected_data = {'fcdo': {'LR': {'15110': 1232 / 0.726269155}}, | ||
"finddiagnostics": {'AO': {'32182': (71189586 / 100) * 0.18 * 0.72, | ||
'12210': (71189586 / 100) * 0.18 * 0.28}, | ||
'AE': {'32182': (71189586 / 100) * 0.01 * 0.72, | ||
'12210': (71189586 / 100) * 0.01 * 0.28}, | ||
'AR': {'32182': (71189586 / 100) * 0.01 * 0.72, | ||
'12210': (71189586 / 100) * 0.01 * 0.28}, | ||
'AT': {'32182': (71189586 / 100) * 0.0001 * 0.72, | ||
'12210': (71189586 / 100) * 0.0001 * 0.28}, | ||
}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Although this works, I think this is slightly confusing because you divide only the non-USD values by 100. Then in the finddiagnostics
file, country AO
is 0.18% (So 0.0018) and sector 32182
is 72% (so 0.72). I think it would be good to just divide by 1 (the USD:USD conversion rate) and then multiply by the fraction each time.
So perhaps this could instead be written as:
{ 'AO': {'32182': (71189586 / 1) * 0.0018 * 0.72 }}
No description provided.