fixed the missing documentation. #3120

deep-poharkar · 2023-02-17T08:32:09Z

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

Immediately
Within one week
When possible

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

My change requires a change to the documentation.
My name is in the list of CITATION.cff
I have updated the CHANGELOG.md.
I have updated the documentation accordingly.
I have read the CONTRIBUTING document.
I have added tests to cover my changes.
All new and existing tests passed.

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.

mdietze

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.

deep-poharkar · 2023-02-17T15:27:43Z

@mdietze can you review the improved one.

modules/data.atmosphere/R/solar_angle.R

mdietze · 2023-02-17T15:41:12Z

modules/data.atmosphere/R/solar_angle.R

+#' 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.


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.

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.

deep-poharkar · 2023-02-17T19:46:24Z

@mdietze what do you think about the updated information.

Aariq · 2023-02-27T17:20:32Z

modules/data.atmosphere/R/solar_angle.R

+#' 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).


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.

Suggested change

#' 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).

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.

Aariq · 2023-02-27T17:25:03Z

modules/data.atmosphere/R/solar_angle.R

@@ -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/


CRAN is picky about URLs—can you run urlchecker::url_check("modules/data.atmosphere") to check that this is OK?

mdietze · 2023-03-01T13:40:56Z

modules/data.atmosphere/R/solar_angle.R

+#' 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")


This seems like it's a reference, not an example, and it duplicates an existing reference so I don't think it is needed

mdietze · 2023-03-01T13:41:07Z

modules/data.atmosphere/R/solar_angle.R

+#' 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).


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.

mdietze · 2023-03-01T13:42:13Z

modules/data.atmosphere/R/solar_angle.R

+#' @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


This reference seems tangential. I'd drop

mdietze · 2023-03-01T13:42:52Z

modules/data.atmosphere/R/solar_angle.R

+#' 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.


Why integer? Code looks like it would be happy to work with fractional hours

mdietze · 2023-03-01T13:45:20Z

modules/data.atmosphere/R/solar_angle.R

+#' 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).


pretty sure it's actually seconds

mdietze · 2023-03-01T13:45:47Z

modules/data.atmosphere/R/solar_angle.R

+#'
+#' 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.


Ok, but you didn't give the function a default

mdietze · 2023-03-01T13:49:14Z

modules/data.atmosphere/R/solar_angle.R

+#' Unit - Hours (h)
+#'
+#' dt Timestep : It represents the time interval between two values.
+#' Therefore it is an integer value ranging from 0 to 60.


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"

mdietze · 2023-03-01T13:50:20Z

modules/data.atmosphere/R/solar_angle.R

+#' 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).


could be shortened to "decimal degrees"

Aariq · 2023-03-01T15:55:48Z

modules/data.atmosphere/R/solar_angle.R

@@ -11,18 +55,18 @@
 #' @return `numeric(1)` of cosine of solar zenith angle
 #' @export


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.

mdietze · 2023-04-26T16:54:13Z

@deep-poharkar pinging you to see if you could finish up this PR

deep-poharkar · 2023-05-04T15:00:02Z

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.

mdietze · 2023-09-03T01:24:09Z

@deep-poharkar pinging you about finishing up this PR

GandalfGwaihir · 2024-05-12T20:37:38Z

are we close to finishing this PR @deep-poharkar ? it's been a year :)

fixed the missing documentation.

9903b1b

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.

mdietze requested changes Feb 17, 2023

View reviewed changes

deep-poharkar and others added 2 commits February 17, 2023 20:53

improved the documentation

9a9cd55

Merge branch 'PecanProject:develop' into develop

6eaab76

mdietze reviewed Feb 17, 2023

View reviewed changes

Added more details to the documentation.

a782da7

Aariq reviewed Feb 27, 2023

View reviewed changes

mdietze reviewed Mar 1, 2023

View reviewed changes

Aariq reviewed Mar 1, 2023

View reviewed changes

Merge branch 'develop' into develop

3c6caa0

Merge branch 'PecanProject:develop' into develop

c588fb2

Sweetdevil144 mentioned this pull request Mar 7, 2024

CRAN Compliance Issue: Broken and Redirected URLs in PEcAn's data.atmosphere #3269

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fixed the missing documentation. #3120

fixed the missing documentation. #3120

deep-poharkar commented Feb 17, 2023 •

edited

mdietze left a comment

deep-poharkar commented Feb 17, 2023

mdietze Feb 17, 2023

deep-poharkar Feb 17, 2023

deep-poharkar commented Feb 17, 2023

Aariq Feb 27, 2023 •

edited

mdietze Mar 1, 2023

Aariq Feb 27, 2023

mdietze Mar 1, 2023

mdietze Mar 1, 2023

mdietze Mar 1, 2023

mdietze Mar 1, 2023

mdietze Mar 1, 2023

mdietze Mar 1, 2023

mdietze Mar 1, 2023

mdietze Mar 1, 2023

Aariq Mar 1, 2023

mdietze commented Apr 26, 2023

deep-poharkar commented May 4, 2023

mdietze commented Sep 3, 2023

GandalfGwaihir commented May 12, 2024

		@@ -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/

		@@ -11,18 +55,18 @@
		#' @return `numeric(1)` of cosine of solar zenith angle
		#' @export

fixed the missing documentation. #3120

Are you sure you want to change the base?

fixed the missing documentation. #3120

Conversation

deep-poharkar commented Feb 17, 2023 • edited

Description

Motivation and Context

Review Time Estimate

Types of changes

Checklist:

mdietze left a comment

Choose a reason for hiding this comment

deep-poharkar commented Feb 17, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deep-poharkar commented Feb 17, 2023

Aariq Feb 27, 2023 • edited

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

mdietze commented Apr 26, 2023

deep-poharkar commented May 4, 2023

mdietze commented Sep 3, 2023

GandalfGwaihir commented May 12, 2024

deep-poharkar commented Feb 17, 2023 •

edited

Aariq Feb 27, 2023 •

edited