Revision of function documentation #36

tomaszaba · 2024-10-12T17:30:52Z

Hi Ernest,

In this PR I addressed the issues #17, #18 , #19.

Please review.

ernestguevarra

@tomaszaba thanks for your hard work on this. You are really putting in so much work over a short period of time. I worry a little bit that this might be impacting on attention to detail and quality from a documentation perspective.

I only got through the age.R function scripts as there are repeating patterns of how you write the technical documentation that I think is not what is considered best practice. You are writing the technical documentation as if the audience knows nothing at all of how a function works and doesn't know how to code. For someone to get to this stage where they are reading your help page, they would at the very least know how to install your package and know how to invoke the help (?) command/function. If they don't know how to do that, then they don't know R and they won't be able to get to your documentation. The use of function documentation is mainly to get help within R for use of a function. Rarely will an R user go to the package website to read the documentation for a function. They will go to the website for README and vignettes.

From that perspective, you need to write your function documentation knowing that your reader is an R user who has basic knowledge of how to use a function.

I think this general comment and the specific comment for the documentation in the age functions can give you an idea of what most of my comments will be for the other function documentation. If you can have a look at my feedback and just review your other function documentation with this in mind that would be great.

I also think that a doing this process in smaller chunks is more productive and sustainable than doing one whole chunk of 74 files being changed and then asking for a review. If we go through a set of files to edit and then review, you'll get a good sense of the style that we can agree on and then from there, you will be able to apply those changes to the rest.

What do you think?

ernestguevarra · 2024-10-12T21:38:57Z

R/age.R

+#' @returns A numeric vector, of the same length as the input variable, containing
+#' age values in days.


Suggest instead to use this:

@returns A numeric vector of same length as `x` of age in days.

A related question is whether age in days is numeric or integer?

As of now I have age in days with two decimal places - reason why I went for numeric. I did this to be consistent with months, which also takes 2 decimal places.

ernestguevarra · 2024-10-12T21:43:25Z

R/age.R

+#' `compute_age_in_months()` calculates age in months from on the basis of
+#' difference between the data collection date and the child's date of birth.
+#' It works inside [dplyr::mutate()] or [base::transform()].


Avoid starting the description statement with the function name. For example, instead of what you have here, you can say:

Calculate child's age in months based on child's date of birth and the date of data collection.

I don't see why we need to say that this function works inside dplyr::mutate?

Because if you call it outside mutating function it will not work. It's just a heads up that I thought I should put there for non-advanced R users.

ernestguevarra · 2024-10-12T21:52:31Z

R/age.R

+#' @param surv_date A vector of class "Date" holding values corresponding to
+#' the date of data collection.


Maybe this is better?

@param surv_date A vector of class Date for date of data collection.

ernestguevarra · 2024-10-12T21:56:17Z

R/age.R

+#' @param birth_date A vector of class "Date" holding values corresponding to
+#' the child's date of birth.


Maybe a similar structure as my suggestion above:

@param birth_date A vector of class Date for child's birth date

ernestguevarra · 2024-10-12T22:05:03Z

R/age.R

+#' @returns A numeric vector named `age` holding age values in months with two
+#' decimal places. Any value outside the range of 6.0 to 59.99 is replaced with
+#' `NA`.


Several comments here.

This is not really a documentation issue but more of the function but is related to documentation because you had to explain it here. Why should a function that is meant to calculate age in months have as one of its features that it removes ages outside a specific range? I know why you are doing that but I am not sure this is the function that should be doing that filtering. It doesn't make sense.

Recommend changing the text to something like this:

@returns A numeric vector for child's age in months

Why the need to round off in this function? More so that this is not even an exported function? What value is there to round off in this function? Why not do this somewhere else - process_age()?

Thanks for this review. Will work on them.

ernestguevarra · 2024-10-13T16:12:28Z

R/age.R

+#' @param svdate A vector of class "Date" holding values corresponding to
+#' the data collection date. Default is `NULL`.


why call this argument svdate and not surv_date as you used in the earlier function? why do these parameters be different?

You say here that svdate is a vector of class Date but what they need to supply here is a character value for the name of the variable in df for survey date? Am I correct or am I missing something?

This is something that I noticed during my revision, but I did not address because it was not the right time; changing that would mean having to touch many files in this branch, including function definitions and tests. I took note of this and will revise. Actually, this is where I want to refactor the function so that those parameters can be called needless to use " ". This is because even me I make mistakes when I call this function. Because I use data masking approach almost everywhere, when I call that function, I tend to pass those parameters without " " and it errors.

Thanks for raising this issue.

ernestguevarra · 2024-10-13T16:13:34Z

R/age.R

+#' @param birdate A vector of class "Date" holding values corresponding to
+#' the child's date of birth. Default is `NULL`.


Same comment here as svdate - why different name from earlier function and isn't what this needs is a character value for the name of variable in df for date of birth?

ernestguevarra · 2024-10-13T16:25:57Z

R/age.R

+#' @returns A data frame of the same length as the input with an additional
+#' column. A new variable, `age_day`, is added to the output data frame whilst
+#' the `age` variable gets filled where applicable, and then any values outside
+#' the range of 6.0 to 59.99 months get replaced with `NA`.


I would rewrite this documentation to:

@returns A data.frame based on df. Variable age that is required to be included in df will be filled where applicable with the age in months for each row of data in df. A new variable for df named age_days will be created. Values for age and age_days for children less than 6 months old and 60 months or more will be set to NA.

ernestguevarra · 2024-10-13T16:33:02Z

R/age.R

+#' @param age A numeric vector holding age values in months, usually estimated
+#' using local event calendars.


Is this really numeric or should this be integer? If this is from local event calendars then this should be integer right?

This is not like the other arguments where you require the quoted variable name, here you use the unqouted variable name. Why the difference? If I were a user and I am faced with this syntax, I will get really frustrated as it doesn't make sense to me why the other parameters have to be quoted while another needs to be unquoted. I Know you like the whole complicated data masking approaches but I think you have to be consistent here. choose quoted or unqouted for all parameters and not mixed

Indeed, I did mention this frustration I experienced myself. I will refactor this and yes, to data masking thing :-) it keeps code clean, I find.

ernestguevarra · 2024-10-13T16:40:50Z

R/age.R

-#' # Have a sample data ----
+#' ## A sample data ----
 #' df <- data.frame(
 #' survy_date = as.Date(c(
 #' "2023-01-01", "2023-01-01", "2023-01-01", "2023-01-01", "2023-01-01")),


Do you really need to make your own data.frame as an example? Can you not use a dataset and include in the package and use it for examples?

Took note of this. Will revise accordingly.

I remember why I had to create my own dataset. My dataset does not have child's date of birth to demonstrate this function properly. In anthro.01 there is only the date of data collection, whilst child's age is all filled with NA. The data came as it is.

tomaszaba · 2024-10-14T05:56:46Z

@tomaszaba thanks for your hard work on this. You are really putting in so much work over a short period of time. I worry a little bit that this might be impacting on attention to detail and quality from a documentation perspective.

I only got through the age.R function scripts as there are repeating patterns of how you write the technical documentation that I think is not what is considered best practice. You are writing the technical documentation as if the audience knows nothing at all of how a function works and doesn't know how to code. For someone to get to this stage where they are reading your help page, they would at the very least know how to install your package and know how to invoke the help (?) command/function. If they don't know how to do that, then they don't know R and they won't be able to get to your documentation. The use of function documentation is mainly to get help within R for use of a function. Rarely will an R user go to the package website to read the documentation for a function. They will go to the website for README and vignettes.

From that perspective, you need to write your function documentation knowing that your reader is an R user who has basic knowledge of how to use a function.

I think this general comment and the specific comment for the documentation in the age functions can give you an idea of what most of my comments will be for the other function documentation. If you can have a look at my feedback and just review your other function documentation with this in mind that would be great.

I also think that a doing this process in smaller chunks is more productive and sustainable than doing one whole chunk of 74 files being changed and then asking for a review. If we go through a set of files to edit and then review, you'll get a good sense of the style that we can agree on and then from there, you will be able to apply those changes to the rest.

What do you think?

@ernestguevarra , I appreciate your comprehensive feedback. I do admit to have followed that approach. This was the guidance I got from Hadley's book, including the mentioning of the package function at the beginning of @description. But at the same time I also used my approach, which I had in mind the non-advanced R users. When you are not an advanced user, which is majority of my audience I believe, writing function documentation in an advanced way can limit their ability to understand. This was my personal experience when started using R. I used to find some function documentation too difficult to grasp. So I decided to work around this in this package using simple language. If you still think we should avoid that styling, I can strive to rework on this to incorporate the styling you are proposing to follow. Please let me know.

Regarding to your last point about chunks of changes in a branch, I do agree but I would like you to propose the strategy to operationalise. Do you suggest changing one .R file then raise a PR?

What about the coherence in the sentences. Has this improved in at least the file you reviewed?

Thanks.

tomaszaba · 2024-10-18T05:15:41Z

@tomaszaba thanks for your hard work on this. You are really putting in so much work over a short period of time. I worry a little bit that this might be impacting on attention to detail and quality from a documentation perspective.
I only got through the age.R function scripts as there are repeating patterns of how you write the technical documentation that I think is not what is considered best practice. You are writing the technical documentation as if the audience knows nothing at all of how a function works and doesn't know how to code. For someone to get to this stage where they are reading your help page, they would at the very least know how to install your package and know how to invoke the help (?) command/function. If they don't know how to do that, then they don't know R and they won't be able to get to your documentation. The use of function documentation is mainly to get help within R for use of a function. Rarely will an R user go to the package website to read the documentation for a function. They will go to the website for README and vignettes.
From that perspective, you need to write your function documentation knowing that your reader is an R user who has basic knowledge of how to use a function.
I think this general comment and the specific comment for the documentation in the age functions can give you an idea of what most of my comments will be for the other function documentation. If you can have a look at my feedback and just review your other function documentation with this in mind that would be great.
I also think that a doing this process in smaller chunks is more productive and sustainable than doing one whole chunk of 74 files being changed and then asking for a review. If we go through a set of files to edit and then review, you'll get a good sense of the style that we can agree on and then from there, you will be able to apply those changes to the rest.
What do you think?

@ernestguevarra , I appreciate your comprehensive feedback. I do admit to have followed that approach. This was the guidance I got from Hadley's book, including the mentioning of the package function at the beginning of @description. But at the same time I also used my approach, which I had in mind the non-advanced R users. When you are not an advanced user, which is majority of my audience I believe, writing function documentation in an advanced way can limit their ability to understand. This was my personal experience when started using R. I used to find some function documentation too difficult to grasp. So I decided to work around this in this package using simple language. If you still think we should avoid that styling, I can strive to rework on this to incorporate the styling you are proposing to follow. Please let me know.

Regarding to your last point about chunks of changes in a branch, I do agree but I would like you to propose the strategy to operationalise. Do you suggest changing one .R file then raise a PR?

What about the coherence in the sentences. Has this improved in at least the file you reviewed?

Thanks.

Hi @ernestguevarra,
I have just pushed my revisions to this PR addressing your reviews. Please see below some notes for your consideration while reviewing this PR:

I opted to use the verb to wrangle over to process. I think I have done this throughout the documentation.
Regarding the class of vectors, as agreed, I considered being specific. However, in some instances, as in recode_muac() where inherent to the purpose of the function, I called it "A vector of class double or integer. This is because if the vector comes in cm, that would be double, otherwise integer. On this, I had to revise my sentences to "A vector of class double of …" instead of "A double vector …". This was to avoid the ambiguity around the word "double" that can be perceived as if we're referring to "duplicated or two times vector".
On the quality scores and classifiers, after I read (again) the SMART manual, I noticed that they use "acceptability" to refer to what I had initially called "quality". In this way, I revised the documentation to the following: "rate" in place of classify and "acceptability" for quality. Rate the acceptability of the standard deviation .
Something awkward is happing with the function in case_definition.R. Somehow the documentation checked with help(define_wasting_cases_whz) returns the argument in a different order that I set in the .R file. . Also, sometimes on the rendered documentation, the chunk on the usage get displayed differently for some functions. I am unaware of what leads to this. Do you know?

ernestguevarra · 2024-10-18T06:56:05Z

R/age.R

I will approve these changes now but I want to re-iterate the following:

"Wrangling child's age" in your function title sounds really funny and bordering on inappropriate....

why different parameter names for different functions for the same inputs? svdate in one and then surv_date in another....

your argument definition for surv_date and birdate is incorrect. What you are asking the input of for that is not the vector itself but the variable name in df that holds the survey date and the birth date. I mentioned this in my previous review and I think this is something that is not about preference but about what is correct. So, I think this should be "A character value for the name of the variable in df containing a vector of values of class Date for the survey date...". There is a big difference between this and your definition. If I were to follow your definition, then I will apply the following in the function:

process_age(input = df, svdate = df$survey_date, ...)

which is not what you want...

Again, I know you might be thinking this is not important but for a user, it is confusing. Please make the specification of age argument in process_age consistent with how the dates are specified. Either have the user declare the variable for age (as is with the dates) are use an unquoted variable for svdate and birdate. I have been using R for close to 20 years now and have read so much documentation. If I read yours for this function, I will make mistakes in specifying dates and age because of how these are done in different ways...We have to be consistent. I mentioned this in my comment and I think you disregarded it. I am not sure why but this along with number 3 above I think is a critical comment that needs to have an action either way.

Hi Ernest,

I decided to change to "wrangle age" for consistency. I thought it would not be right to use the verb to wrangle here and to process there. If that is Ok for you I can put it back to using the verb to process on age.

The issue around different parameters, which you had raised in your previous reviews, are well noted and listed for action in the next brach where I will be re-factoring the functions. If I had revised those parameters that would have entailed to change function definitions, test files. I have listed all these issues, and others that I noticed, to address in the branch about function definitions. I thought this was the right approach to follow. I should have mentioned this in my message earlier today.

ernestguevarra

@tomaszaba happy to approve this now. You've done really hard work on good job on this.

I think we need to get your hands and brains working on re-factor and actual coding rather than just docs at this stage. I think reviewing the docs helped you see what kind of re-factor you need. I am very confident that you have that mindset now.

So, it's important we let you do code now! which is always the fun stuff!!!!

👍🏾 🎉

tomaszaba added 4 commits October 11, 2024 21:20

revise functions documentation

045eb85

revise function docs 2

3144f11

revise function doc 3

1a1b1e0

revise function doc 4

a1ce73b

tomaszaba added the documentation Improvements or additions to documentation label Oct 12, 2024

tomaszaba requested a review from ernestguevarra October 12, 2024 17:30

tomaszaba assigned ernestguevarra Oct 12, 2024

tomaszaba changed the title ~~Revision of the functions documentation~~ Revision of function documentation Oct 12, 2024

This was linked to issues Oct 12, 2024

ROXYGEN: shorten @description and add details in @detail and in @section #17

Closed

ROXYGEN: use one @param for every argument #18

Closed

ROXYGEN: make use of @Rdname to group functions into commeon Roxygen #19

Closed

ernestguevarra requested changes Oct 13, 2024

View reviewed changes

tomaszaba added 5 commits October 17, 2024 14:32

doc: revise function doc (1)

8fd6f70

doc: revise doc (plausibility)

2972191

doc: revise doc (case-definition)

fe30649

doc: revise doc (prevalence)

f5d1afd

doc: revise doc (data)

49077d1

ernestguevarra reviewed Oct 18, 2024

View reviewed changes

ernestguevarra approved these changes Oct 18, 2024

View reviewed changes

ernestguevarra merged commit 74837a1 into main Oct 18, 2024
7 checks passed

tomaszaba deleted the function_doc branch November 3, 2024 21:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Revision of function documentation #36

Revision of function documentation #36

tomaszaba commented Oct 12, 2024

ernestguevarra left a comment •

edited

Loading

ernestguevarra Oct 12, 2024 •

edited

Loading

tomaszaba Oct 14, 2024

ernestguevarra Oct 12, 2024

tomaszaba Oct 14, 2024

ernestguevarra Oct 12, 2024

ernestguevarra Oct 12, 2024

ernestguevarra Oct 12, 2024

tomaszaba Oct 14, 2024

ernestguevarra Oct 13, 2024

tomaszaba Oct 14, 2024

ernestguevarra Oct 13, 2024

ernestguevarra Oct 13, 2024

ernestguevarra Oct 13, 2024

tomaszaba Oct 14, 2024

ernestguevarra Oct 13, 2024

tomaszaba Oct 14, 2024

tomaszaba Oct 15, 2024

tomaszaba commented Oct 14, 2024

tomaszaba commented Oct 18, 2024

ernestguevarra Oct 18, 2024 •

edited

Loading

tomaszaba Oct 18, 2024 •

edited

Loading

ernestguevarra left a comment

		#' @returns A numeric vector, of the same length as the input variable, containing
		#' age values in days.

		#' @param surv_date A vector of class "Date" holding values corresponding to
		#' the date of data collection.

		#' @param birth_date A vector of class "Date" holding values corresponding to
		#' the child's date of birth.

		#' @param svdate A vector of class "Date" holding values corresponding to
		#' the data collection date. Default is `NULL`.

		#' @param birdate A vector of class "Date" holding values corresponding to
		#' the child's date of birth. Default is `NULL`.

		#' @param age A numeric vector holding age values in months, usually estimated
		#' using local event calendars.

Revision of function documentation #36

Revision of function documentation #36

Conversation

tomaszaba commented Oct 12, 2024

ernestguevarra left a comment • edited Loading

Choose a reason for hiding this comment

ernestguevarra Oct 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomaszaba commented Oct 14, 2024

tomaszaba commented Oct 18, 2024

ernestguevarra Oct 18, 2024 • edited Loading

Choose a reason for hiding this comment

tomaszaba Oct 18, 2024 • edited Loading

Choose a reason for hiding this comment

ernestguevarra left a comment

Choose a reason for hiding this comment

ernestguevarra left a comment •

edited

Loading

ernestguevarra Oct 12, 2024 •

edited

Loading

ernestguevarra Oct 18, 2024 •

edited

Loading

tomaszaba Oct 18, 2024 •

edited

Loading