-
Notifications
You must be signed in to change notification settings - Fork 15
Add Costing Resource Files and Scripts #1637
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
Conversation
…to the master facilities list for costing
- incremental input cost was multiplied by (1+_non_zero_implementation_cost_proportion) but it should be multiplied by _non_zero_implementation_cost_proportion. Note that the ROI estimates are still correct.
|
Hi @tbhallett. I've now closed the old costing PR (PR #1279) and moved the relevant scripts and resource files here. This PR is now ready for review. I have added a README.md file to describe the utility functions for costing. Happy to move this to a Wiki if that's preferable. One test is failing. It seems to be related to the test files (not part of the costing scripts): |
Thanks @sakshimohan -- will review this very soon |
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 is the key file containing all the functions that we expect to be re-used and which we want to make sure always works.
Given this, I propose moving this file to src\tlo\analysis\
| _discount_rate: Union[float, dict[int, float]] = 0, | ||
| _initial_year: Optional[int] = None, | ||
| _column_for_discounting: str = 'cost') -> pd.DataFrame: |
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.
here, and elsewhere, arguments are named with the leading underscore.
Whilst this all works perfectly, the convention is for "_" to be used for private members, and so it's unusual to have them as argument names.
https://betterstack.com/community/questions/what-does-underscores-before-object-name-mean-in-python/
So, to improve future readability, I would propose a refactor to just rename the arguments to be without the leading underscore.
|
Noting here that this change does cause a fail on |
|
Looks like it's |
|
Some unresolved comments above - are you happy to merge and deal with those in a later PR? |
yes, we've agreed that those points can be recorded as an issue for future consideration. |
This PR adds the following -
cost_estimation.py) and related resource files (~/resources/costing/)For more information, see
~src/scripts/costing/README.md.Note that these files were extracted from a previous [PR #1279], which was closed due to conflicts with master.
Possible future improvements to the costing module are listed under Issue #1635