148 Add EQ5D as a person attribute#151
Conversation
…-a-person-attribute
|
Why is the variable called deq5d? I expected this to be a health variable, so the naming convention would suggest heq5d. the d prefix would suggest a dummy or demographic variable. The same point is relevant to the mcs and pcs variables. Do we want non-zero values on the diagonal of the VCM? This breaks the link to the SF-12 component scores where parameter uncertainty has already been captured. This process is a simple conversion of an estimated outcome, not a new estimation. It is worth keeping the VCM and the code structure to allow other users to make a different decision, but I would suggest a zero matrix as the base. I would suggest that Erik and one of the Essex team are better placed to review the tests. Great that they have been added. |
Good point. I had copied other similar variables such as Also, if it's helpful to be more verbose in internal variable and method naming, is a fuller name of |
|
I would suggest that dhm is poorly named and we should not repeat this convention throughout the code. It is likely that more health outcomes will be added in subsequent project, so dealing with this issue now seems prudent. Ideally, we would change the dhm, dhm_ghq, and dhe names, but this may be risky given how widely they are used in the code. My suggestions would be: |
|
For renaming, my suggestion would be to then rename these as |
igelstorm
left a comment
There was a problem hiding this comment.
I've made some specific suggestions, but broadly this looks good!
What
dhe_mcsanddhe_pcs) to EQ5D utility indexPersonTest.javafile as tests for this calculation (and placeholder for further Person tests)Why