-
Notifications
You must be signed in to change notification settings - Fork 164
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
F npi ndc bugfix #164
F npi ndc bugfix #164
Conversation
- flows were only calculated for the last out of all policy countries - time period lengths were mapped incorrectly on the data - rewrote scripts for better performance (new scripts are approx 4x faster)
…f_npi_ndc_bugfix
can you give me a sign as soon as it is ready for review? |
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.
Quite some changes. Looks all plausible to me. I will approve and make a test.
@flohump I've made tests already and there is virtually no effect on the results. I expect your test to confirm the same. However, the calculation of the npi/ndc AD policy contains a different type of a bug, i.e. the minimum forest stock constraint is wrongly calculated with respect to what counties actually promise. I'm currently reviewing the policies and changing the code accordingly. The question is: should this merge get reverted or we keep it? It certainly speeds up the calculation, but correction to the AD policy should be done. |
Contains changes in the start_npi_ndc.R script that were introduced by Jan (correction of flows calculation and time period length; and the improvements for performance), and small fixes for paths corrected by me.
The script is under further development, mainly to revise the calculation for the AD policy, give that the resulting forest lower bounds are inconsistent with AD policy definitions for some regions.