-
Notifications
You must be signed in to change notification settings - Fork 234
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fixed the missing documentation. #3120
base: develop
Are you sure you want to change the base?
Conversation
I tried to fix (Broken link and missing documentation for cos_solar_zenith_angle() PecanProject#3084) by providing description, references and example. The given in link in description was working but I can update that if needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't really address the core problems raised in #3084 (e.g. dt vs hr, what are the units on these things? which things are scalars vs vectors?) but instead just reiterates information that's already present in a slightly different (but not necessarily better) format.
FWIW, I don't know if someone already fixed the link issue mentioned in #3084, but that doesn't seem to be a problem for me.
@mdietze can you review the improved one. |
#' lat Latitude : Latitude measures the distance north or south of the equator. The primary unit of latitude is degrees (°). | ||
#' lon Longitude : Longitude measures distance east or west of the prime meridian. The primary unit of longitude is degrees (°). | ||
#' dt Timestep : Timestep is a scalar quantity. | ||
#' hr Hours timestep : It is a timestep in hours format. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still doesn't really explain what dt and hr actually are and what acceptable values are. Looking at the code, hr isn't a "timestep" it's the time of day that you're trying to predict zenith angle for (If I'd written this function I'd have put it next to doy to make this connection more obvious). Looking at the code I'd guess this should be a number between 0-24 and that decimal values are acceptable (i.e. one should use fractional hours instead of minutes). I agree dt is probably a scalar, but you likewise still haven't said what it actually is or what acceptable values are. My best guess it that the author of the function anticipated that many people would be passing in vectors of days and hours, and thus if you have a whole vector of times you might actually want to do the calculation for the midpoint between two timesteps. dt thus tells the function how far apart values in hr are. Seeing that this is calculated by dividing dt by 86400 and then multiplying by 24, it looks like dt needs to be in SECONDS. In my mind dt should probably be given a default of 0.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure, I have made the changes in the code and made it more in detail. I have one doubt should I write an example, if so then it'll take too many lines.
@mdietze what do you think about the updated information. |
#' It depends upon the following - | ||
#' | ||
#' doy Day of year : It is the day when you are calculating the angle. | ||
#' January 1st being day 1 and December 31st being day 365 (or 366 in leap years). | ||
#' It is used to determine the position of the sun in the sky throughout the year. | ||
#' Unit - Dimensionless (a simple count of the day within a year). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use roxygen2 tag @param
for documentation of argument names. Run devtools::document("modules/data.atmosphere")
and check the help file (?cos_solar_zenith_angle
) to see how it is formatted.
#' It depends upon the following - | |
#' | |
#' doy Day of year : It is the day when you are calculating the angle. | |
#' January 1st being day 1 and December 31st being day 365 (or 366 in leap years). | |
#' It is used to determine the position of the sun in the sky throughout the year. | |
#' Unit - Dimensionless (a simple count of the day within a year). | |
#' | |
#' @param doy Day of year : It is the day when you are calculating the angle. | |
#' January 1st being day 1 and December 31st being day 365 (or 366 in leap years). | |
#' It is used to determine the position of the sun in the sky throughout the year. | |
#' Unit - Dimensionless (a simple count of the day within a year). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Further clarification: you already have a @param
tag for every variable, and you don't want to create duplicates. Instead, you want to add the basic variable-by-variable details to the existing variable descriptions (which are too short). Beyond that, if you need to provide a more detailed explanation of what the function is doing, it should go in a @details
section, not in the @description
, which should be no more than a few sentences.
@@ -2,6 +2,50 @@ | |||
#' | |||
#' For explanations of formulae, see https://web.archive.org/web/20180307133425/http://www.itacanet.org/the-sun-as-a-source-of-energy/part-3-calculating-solar-angles/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CRAN is picky about URLs—can you run urlchecker::url_check("modules/data.atmosphere")
to check that this is OK?
#' https://wiki.seas.harvard.edu/geos-chem/index.php/Centralized_chemistry_time_step | ||
|
||
#' @examplesIf interactive() | ||
#' browseURL("https://www.ecmwf.int/sites/default/files/elibrary/2022/20336-calculating-cosine-solar-zenith-angle-thermal-comfort-indices.pdf") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems like it's a reference, not an example, and it duplicates an existing reference so I don't think it is needed
#' It depends upon the following - | ||
#' | ||
#' doy Day of year : It is the day when you are calculating the angle. | ||
#' January 1st being day 1 and December 31st being day 365 (or 366 in leap years). | ||
#' It is used to determine the position of the sun in the sky throughout the year. | ||
#' Unit - Dimensionless (a simple count of the day within a year). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Further clarification: you already have a @param
tag for every variable, and you don't want to create duplicates. Instead, you want to add the basic variable-by-variable details to the existing variable descriptions (which are too short). Beyond that, if you need to provide a more detailed explanation of what the function is doing, it should go in a @details
section, not in the @description
, which should be no more than a few sentences.
#' @references | ||
#' https://www.ecmwf.int/sites/default/files/elibrary/2022/20336-calculating-cosine-solar-zenith-angle-thermal-comfort-indices.pdf | ||
#' https://centaur.reading.ac.uk/104581/ | ||
#' https://wiki.seas.harvard.edu/geos-chem/index.php/Centralized_chemistry_time_step |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This reference seems tangential. I'd drop
#' Unit - Dimensionless (a simple count of the day within a year). | ||
#' | ||
#' hr Hours timestep : It is the time of day that you're trying to predict zenith angle for. | ||
#' It's an integer value ranging from 0 to 24. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why integer? Code looks like it would be happy to work with fractional hours
#' It is designed to calculate the midpoint between two time steps if value is passed in vectors of days and hours. | ||
#' dt tells the function how far apart values in hours are. | ||
#' a day time step might be used to represent the change in solar position and angle between consecutive days. | ||
#' Unit - Minutes (min). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pretty sure it's actually seconds
#' | ||
#' dt Timestep : It represents the time interval between two values. | ||
#' Therefore it is an integer value ranging from 0 to 60. | ||
#' It should have a default value of 0. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, but you didn't give the function a default
#' Unit - Hours (h) | ||
#' | ||
#' dt Timestep : It represents the time interval between two values. | ||
#' Therefore it is an integer value ranging from 0 to 60. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- doesn't have to be an integer
- I don't think you need to specify the range
- the "between two values" comment doesn't make sense if you are feeding the functions scalar values, which is what a lot of people would assume you do. I think it would be better to add something like "Used if you are passing the function a vector times and want to calculate radiation at the midpoint between timesteps"
#' Unit - Minutes (min). | ||
#' | ||
#' lat Latitude : Latitude measures the distance north or south of the equator. | ||
#' Unit - Degrees (°) in decimal format (e.g., 37.7749° for San Francisco). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could be shortened to "decimal degrees"
@@ -11,18 +55,18 @@ | |||
#' @return `numeric(1)` of cosine of solar zenith angle | |||
#' @export |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
usually @examples
goes at the end of the roxygen2 comments block. There's no hard rule about that, but since examples appear last in rendered documentation, it makes sense to put them last here as well.
@deep-poharkar pinging you to see if you could finish up this PR |
hey @mdietze sorry for replying late. I have been busy with my exams recently. I'll surely try to finish this up by next week. |
@deep-poharkar pinging you about finishing up this PR |
are we close to finishing this PR @deep-poharkar ? it's been a year :) |
Description
I tried to fix (Broken link and missing documentation for cos_solar_zenith_angle() #3084) by providing description, references and example. The given link in description was working but I can update that if needed. I want you to review and tell me what exactly needs to be changed. I am not sure about what should I write in @example
Motivation and Context
This will fix the #3084 which will improve the documentation.
Review Time Estimate
Types of changes
Checklist: