search

best-cost / best-cost_WPs

This repository aim to collect the confidential (not public) work of the EU project BEST-COST in the framework of the workpackages (WPs).
1 stars 0 forks source link

Is the deactivated line of code (filter of year in yld case) needed? #302

Closed ungatoverde closed 2 months ago

ungatoverde commented 2 months ago

I have deactivated this line of code in get_death_yll_yld( ). https://github.com/best-cost/best-cost_WPs/blob/7a4dd38e3fee855390f9ee382d31db20b1943e61/r_package/bestcost/R/get_deaths_yll_yld.R#L156

I did it because

  1. it created a warning
  2. it does not seem to be necessary

After deactivating the line of code the testing script seems to work properly and provide the right results. Looking into the deep interim results, I found that the filter was non-sense because the maximum year of the table to be filtered was lower than the year in the filter.

@luytax Could you please confirm that the filter was not needed? If the filter was not needed, I suggest to merge the yll and yld if statement if(outcome_metric %in% c("yll", "yld")).

https://github.com/best-cost/best-cost_WPs/blob/7a4dd38e3fee855390f9ee382d31db20b1943e61/r_package/bestcost/R/get_deaths_yll_yld.R#L143-L160

Otherwise, please reactivate the line of code ensuring that it does not provide any warning in any function of the testing script.

luytax commented 2 months ago

I think it's needed. This line https://github.com/best-cost/best-cost_WPs/blob/7a4dd38e3fee855390f9ee382d31db20b1943e61/r_package/bestcost/R/get_deaths_yll_yld.R#L156 is the only time during the calculations that the duration of the health outcome is considered.

Now that it's commented out, the duration value provided by the user is not considered: e.g. a health outcome A with duration 1 year and health outcome B with duration 10 years result in the same health impact (which is not correct). Checked that by varying the duration input value in the chunk YLD lifetable in the testing script: no matter the duration value, the output is always the same.

ungatoverde commented 2 months ago

@luytax Did you deactivate the line when you checked that changing the duration does not change the health impact?

ungatoverde commented 2 months ago

This line is the only time during the calculations that the duration of the health outcome is considered.

Well it sounds like the line is needed :-)

I suggest to have an if statement for the filter that includes duration (if outcome_metric %in% "yld") and to summarize for both outcome metrics (if outcome_metric %in% c("yld", "yll")). In this way, we avoid the repetition of a line of code that is common for both cases. @luytax : Do you agree? If you do, who of us should implemented?

luytax commented 2 months ago

@ungatoverde I agree and will implement it