Post escaide fix - Githubissues

zkamvar commented 5 years ago

This fixes some issues that were brought up at ESCAIDE.

zkamvar commented 5 years ago

I've made some changes to the case study that include changing some variable names and re-running the case study so that it uses the most recent version of the incidence package.

@aspina7, do you want to give food back?

aspina7 commented 5 years ago

yep will go through now...

aspina7 commented 5 years ago

couldnt comment directly so just chucking line references (for the pull numbers) on here, some comments are just a wording/typo/suggestion thing so if limited on time feel free to ignore these, will tag them with [TEXT] - so can skip over them.

[x] L46 [TEXT]: typo missing ci in municipality
[x] L50 [TEXT]: senT instead of senD
[x] L56 [TEXT] "in the investigation" can be removed, just leave to assess...
[x] L66 [TEXT]: should add after the brackets, "with onset between...."
[x] L81 [TEXT]: pedantic, but you could just put "including how to:" and then remove all the "how to"s from the bullet points.
[x] L87: conventionally would say "which food items were associated with disease"
[ ] L99: Maybe just post a screenshot instead of this or as well as this? (probably a windows screenshot)
[x] L149: Worth noting that packages only need to be installed once and are then saved on your computer.
[x] L164: Then here note that you need to load packages each time you open a new r session (but not re-install)
[x] L221 [TEXT]: maybe rephrase to "save the data within R as a new object called stegen"
[x] L228: Add a note that "here" just pulls together a character string of the path. Maybe say something like "if you look at what is in the path_to_data object you will notice....
[x] L235: Maybe note here that the path_to_data object is the character string above - and you could similarly have typed the path within the read_excel funciton, but that this is not good data practice...
[x] L260: Maybe worth noting further up that they should be opening from an Rproj - and double check they are in one by looking at the top right corner (lots of people didnt do this and then got confused lower down...
[x] L298: worth introducing here the use.na = "always" option from table? Lots of people asked how to include the NAs
[x] L348: worth mentioning that recode is simply applying labels to our variable, while R still interprets it as 0/1?
[x] L393 [TEXT]: maybe dont use the word test here? Epis likely to confuse with a statistical test. Maybe just say returns TRUE/FALSE if elements of a variable are equal a value or argument you supply. (Or something...)
[x] L397 [TEXT]: here too maybe not use the word test
[x] L407 [TEXT]: maybe change "toy_vector" and "toy_logical" to "example_vector" and "example_logical" - dont think everyone would realise that toy is just an example particularly if english levels not high
[ ] L430: Think probably just remove the dir.create stuff?? Confused people quite a lot and easy to just make a new folder by clicking (also you wouldnt really keep dir.create in your script would you? guess returns an error if folder already exists??)
[ ] L450: Cant we just use the base r version for csv?
[x] L461: Maybe worth nothing that you need to define your filename within the "here" function
[x] L499 [TEXT]: maybe specify that FUN is just where you choose your function (and that this can be any function), couple people were confused about what FUN is...
[ ] L544[TEXT]: maybe easy to make reference to bin as the width based on number of units on the x axis.
[x] L599: More pedantic users would say that technically this is not "incidence" - as strictly speaking it would normally be expressed as a proportion or a rate... Way around that would be to say that it is plotting counts of incident cases (as opposed to prevalent cases)
[ ] L618 [TEXT]: is this stratifying though? Pedantic but perhaps just say colouring by... stratifying would be more like facetwrapping no?
[x] L625: maybe specify that you can choose colours by name and by code (and add a link to those colour chart PDFs) - ppl got thrown off by the codes...

Boarding now, will come back to this, poke me if i forget

aspina7 commented 5 years ago

Not sure why that was bold, but very well...

[x] L644: Worth adding explanations for the various different additions, perhaps just highlighting the axis text bits, and that coord_equal is what makes it boxy
[x] L663 [TEXT]: This is where I would use the word stratify as opposed to in L618...
[x] L682: maybe instead of saying which was responsible for the outbreak - say which was associated with illness? (correlation vs causation)
[x] L709: You need to update the two paragraphs describing the exposure variables here (sorry came back to this after reading the code updates lower down - code fine, description wrong). So this whole only keeping the variables (unless theres a good data practice and/or a rational for using it within apply) - I dont think is necessary. From an epi standpoint you generally would always keep the data that you have collected (because if its not useful you shouldnt collect it). Furthermore when transitioning to multivariable analysis, we would want to include all variables of interest not just food. So, I would suggest: keep the food vector with the names of variables of interest, but dont subset the dataset - just use the food names vector in the functions below?
[ ] L774 [TEXT]: I like to explain lists as a collection of other R objects, so say the list is a paper tray and then each dataframe is a named piece of paper in that tray. Then that the dataframe is two dimensions (rows x columns) and putting that in a list simply adds a third dimension (piling papers on top of each other). Theres a slide in the MSF outbreak course lectures.
[x] L812 [TEXT]: worth repeating here that rows/columns can be referred to either by numbers or names?
[x] L864: change to: "test if the disease is associated with any other variables of interest".
[x] L869: is illness associated with age (would be how epis would be happy expressing it - though same meaning...)
[x] L888: illness does not appear to be associated with age
[x] L892: associated with gender
[x] L918: would stay consistent and say associated here to rather than related
[ ] L927 [TEXT]: pedantic here but if you have a cohort (i.e. everyone) then you are calculating the true risk ratio not estimating it - but I guess considering you dont have 100% response... estimating fine...
[x] L1007: Maybe here just repeat that we use the squarebrackes to select the variables in food from stegen.
[x] L1053: Technically martha is correct here, the wide confidence intervals are due to sample size, it is the overlapping of (significant) confidence intervals that suggests foods are not independently associated with illness.
[ ] L1080: maybe change this from being multi to something else, people were confused thinking that it was a multivariable risk ratio, i.e. corrected for other variables.
[ ] L1100 [TEXT]: and usually define these at the begining of your script no?
[ ] L1190: Just double checking - but shouldnt we introduce "!" as not earlier on - e.g. when introducing subsetting and rules for that?
[ ] L1206: I think you shoul dbe able to remove this projection and it should default to WGS84 (which is what should be using...) - if not then might need to address this to make sure both have correct projection

Fin :)

thibautjombart commented 5 years ago

@aspina7 wow that's super detailed feedback thanks! We should get you PR-ready so that next time you can PR that directly.

@zkamvar do you think you could get that in this week?

zkamvar commented 5 years ago

yup

On Dec 3, 2018, at 18:03 , Thibaut Jombart notifications@github.com wrote:

@aspina7 https://github.com/aspina7 wow that's super detailed feedback thanks! We should get you PR-ready so that next time you can PR that directly.

@zkamvar https://github.com/zkamvar do you think you could get that in this week?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/reconhub/learn/pull/53#issuecomment-443807577, or mute the thread https://github.com/notifications/unsubscribe-auth/ADeIlm7pLWH_3HzAuL-9mJzK5oM33fRHks5u1Wd7gaJpZM4Ywzjz.

zkamvar commented 5 years ago

Not sure why that was bold, but very well...

@aspina7, they were bold because at the end of the list you had:


<!-- snip  -->

L1206: I think you shoul dbe able to remove this projection and it should default to WGS84 (which is what should be using...) - if not then might need to address this to make sure both have correct projection
--------- 
Fin :)

Markdown interpreted that dividing line as a level 2 header. I inserted newlines before and after to make it a horizontal line and added dashes to make the line numbers list elements like so:


<!-- snip  -->

- L1206: I think you shoul dbe able to remove this projection and it should default to WGS84 (which is what should be using...) - if not then might need to address this to make sure both have correct projection

--------- 

Fin :)

aspina7 commented 5 years ago

ah nice! thanks - sorry :)

zkamvar commented 5 years ago

It's a super common mistake! This is why I try to get people to space things out (both vertically and horizontally) in their markdown documents.

zkamvar commented 5 years ago

Thanks for the thorough review, @aspina7. I've made most of the changes you requested with a few exceptions listed here:

(note, all the line numbers from the review are in reference to the file at commit 250d23c4bd4e61ad4132448b39e958438800230f)

L99: Maybe just post a screenshot instead of this or as well as this? (probably a windows screenshot)

This would be helpful. The only thing that I can see being a bit of a problem is the fact that you can't show all of the files at once on a Windows view. I've added some screenshots of RStudio before and after setup to help with the visualisation.

L430: Think probably just remove the dir.create stuff?? Confused people quite a lot and easy to just make a new folder by clicking (also you wouldnt really keep dir.create in your script would you? guess returns an error if folder already exists??)

I have personally used dir.create() in my scripts before. It doesn't throw an error if the folder already exists; it throws a warning. We could have them set up the clean_data/ directory beforehand manually, but that runs the risk of someone skipping over it and having things fail when they want to write their clean data. Besides, doing things like this is good practice for them to work abstractly.

L450: Cant we just use the base r version for csv?

Yes, but it has terrible defaults. The biggest problem with it is that it writes the row names to a separate, unnamed column by default, making it irritating to read in if you forget to include row.names = FALSE.

L544[TEXT]: maybe easy to make reference to bin as the width based on number of units on the x axis.

I'm not exactly sure how to word this, so I left it alone.

L618 [TEXT]: is this stratifying though? Pedantic but perhaps just say colouring by... stratifying would be more like facetwrapping no?

The data themselves are still being stratified; so, yes, it is still stratifying

L774 [TEXT]: I like to explain lists as a collection of other R objects, so say the list is a paper tray and then each dataframe is a named piece of paper in that tray. Then that the dataframe is two dimensions (rows x columns) and putting that in a list simply adds a third dimension (piling papers on top of each other). Theres a slide in the MSF outbreak course lectures.

I like the paper tray analogy, but it's a bit incomplete since a data frame is a list in and of itself. A better example may be a toolbox (various sizes) vs a variety pack of beer (uniform size, but still different values)?

L927 [TEXT]: pedantic here but if you have a cohort (i.e. everyone) then you are calculating the true risk ratio not estimating it - but I guess considering you dont have 100% response... estimating fine...

If we are calculating the true risk ratio, what is the need for confidence intervals, then?

L1080: maybe change this from being multi to something else, people were confused thinking that it was a multivariable risk ratio, i.e. corrected for other variables.

I just went ahead and removed the whole thing. As I was nearing the end of writing it, I realized that I was really uncomfortable with the way I had set this up.

L1100 [TEXT]: and usually define these at the begining of your script no?

See above

L1190: Just double checking - but shouldnt we introduce "!" as not earlier on

e.g. when introducing subsetting and rules for that?

We could introduce it earlier by modifying the logical operator thing, but it seems a bit out of place there.

L1206: I think you shoul dbe able to remove this projection and it should default to WGS84 (which is what should be using...) - if not then might need to address this to make sure both have correct projection

I've tried removing it, but ended up getting an error. Shapefiles are definitely not my forte.

zkamvar commented 5 years ago

@aspina7, the practical is ready for you to review: https://deploy-preview-53--reconlearn.netlify.com/post/stegen.html

aspina7 commented 5 years ago

Nice looks good! agree with comments and omitions too.

Re rr estimates .... touché.

Will look at the projection stuff when i find time

zkamvar commented 5 years ago

I think that I will merge this since the next workshop is days away. If changes are warranted, make another PR

reconhub / learn

Post escaide fix #53