Review of TrendsByMonth and units articles
Package Review
This review is limited to the TrendsByMonth and units articles. I did not assess the overall package documentation and functionality.
- Briefly describe any working relationship you have (had) with the package authors. I have discussed the package and its development with the authors several times.
-
As the reviewer I confirm that there are no conflicts of interest for me to review this work.
Estimated hours spent reviewing: 2
-
Should the author(s) deem it appropriate, I agree to be acknowledged as a package reviewer ("rev" role) in the package DESCRIPTION file.
Review Comments
TrendsByMonth article
- The trends by month article uses Conowingo.PO4.RData, but this dataset is not included in the EGRET package, so I substituted the Choptank data:
eList <- Choptank_eList
pairResults <- runPairs(eList, windowSide = 0,
paStart = 1, paLong = 12,
year1 = 1980, year2 = 2010)
- I suggest adding a '+' to change estimates that are positive. I assume decreases are denoted with a '-'. In my example, one line in the result would be: Concentration v. Q Trend Component +50 %
- I don't see the "byMonth" attribute in pairResults. Is 3.0.7.2 on CRAN? I only see the version as 3.0.7.
- Some typos in this passage: 'To see the tabular output of the results, use the “byMonth” attribute from the runPairs result. Here we show a few years of the months output:'
- The plotMonthTrend function is not defined in the article, so if the function is going to be included in the EGRET package I would revise the next passage to read: 'Next, let’s plot the results using the plotMonthTrend function:' Otherwise, define the function in the article.
- Because the plotMonthTrend function was not defined, I couldn't review it.
units article
- Grammar edits in this sentence: Each “slot” in this qUnit object is required, and the qShortName, qUnitName, unitUSGS, and prefix must be characters.
- All code with the
new()
function gives an error (below). Might be related to the EGRET version mentioned above.
Error in initialize(value, ...) :
invalid name for slot of class “qUnit”: prefix
- Under 'Data Input', edit first sentence to read: 'Discharge data in units other than m^3/s can be used as input in EGRET, but that data will be converted to m^3/s with the qUnit argument when the eList is created.'
If the problems I had with the code are related the EGRET version and you can point me to the correct version, I can redo this review.