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

transaction calculations: fixes #9 #20

Merged
merged 2 commits into from
Nov 14, 2023
Merged

transaction calculations: fixes #9 #20

merged 2 commits into from
Nov 14, 2023

Conversation

simon-20
Copy link
Contributor

@simon-20 simon-20 commented Nov 8, 2023

No description provided.

@simon-20 simon-20 requested a review from markbrough November 8, 2023 12:03
Copy link
Contributor

@markbrough markbrough left a 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()])
Copy link
Contributor

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.

Comment on lines +135 to +144
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},
}}
Copy link
Contributor

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 }}

@simon-20 simon-20 merged commit b256026 into iati-data-access:main Nov 14, 2023
1 check passed
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

Successfully merging this pull request may close these issues.

2 participants