search

Public-Health-Scotland / phsmethods

An R package to standardise methods used in Public Health Scotland (https://public-health-scotland.github.io/phsmethods/)
https://public-health-scotland.github.io/phsmethods/
54 stars 13 forks source link

Correct version of SIMD for a given year #29

Closed chrisdeans closed 2 years ago

chrisdeans commented 4 years ago

As part of the QI toolkit group, I've started putting together a function to match on SIMD by postcode/datazone, choosing the correct version of SIMD based on the year. I've created a branch for this: simd_lookup.

davidc92 commented 4 years ago

Hi Chris

Can I ask that in future you ensure you follow the process outlined under "Contributing to phsmethods" in the readme? Step one is opening an issue and awaiting approval from myself/ @lucindalawrie/ @jackhannah95. The QI group isn't pre-approval. This is to aid us in managing the admin side of things and to reduce duplication (e.g. #23 was effectively a duplicate as there was a branch, but no corresponding issue).

I've had a look at the code and whilst I appreciate that it is obviously not complete, it looks a bit overly complicated to me for something that should be relatively simple. I agree with the principle of the function (although will let @lucindalawrie and @jackhannah95 chip in their own thoughts there), but would have a lot of comments in its current form. Before you request a formal review, could you do some work to simplify? Just some things to bear in mind: you should maybe combine everything into the one script file rather than having separate ones for each function. I also think is.pc7() and is.pc8() is a bit redundant since you could just run the postcode function on the input? Food for thought.

David

chrisdeans commented 4 years ago

Hi David,

Sorry, I thought this was what you had asked me to do in the QI toolkit meeting, but clearly I misunderstood. Do you want me to keep the branch as is or should I remove it?

I think you're right that it could be a lot simpler. I'll have a look at doing this over the next few weeks.

Thanks, Chris

chrisdeans commented 4 years ago

Hi @davidc92, @jackhannah95 and @lucindalawrie, I've updated the branch for this following David's suggestions. I've also added documentation, though I haven't started looking at tests yet.

Do you think something along these lines would be worth including in the package?

jackhannah95 commented 4 years ago

Hi @chrisdeans. I'm not opposed to this in theory, although I haven't looked at your code in great detail. I think getting this reviewed and approved within the next fortnight might be a bit optimistic, and for that reason could I ask that you maybe hold off until the next release of the package is out?

I've taken over Jaime's branch, and once that's merged there'll be something of a template for how to save, store and refer to data that comes bundled as part of the package. It doesn't look like you're too far off at the moment, but it likely will necessitate some slight changes on your part.

I've also been editing some other files in that branch that you've edited (notably postcode) and it's likely to led to a few merge conflicts if you try to merge the existing branch. I think I would advise taking a copy of the code you have now, removing the current branch for the time being and re-opening it after the 0.2.0 release for David and/or Lucinda to do a review. The whole experience will probably be a bit less painful for everyone that way.

chrisdeans commented 4 years ago

Hi @jackhannah95, no problem, I've removed the branch.