search

DOI-USGS / loadflex

Models and Tools for Watershed Flux Estimates
http://dx.doi.org/10.1890/ES14-00517.1
Other
14 stars 17 forks source link

first take at expanding predictSolute #219

Closed wdwatkins closed 7 years ago

wdwatkins commented 7 years ago

199

One warning is for the vignette, there is also an existing S3 consistency warning from predictSolute.loadComp.

aappling-usgs commented 7 years ago

I see this S3 consistency warning in the Appveyor build:

* checking S3 generic/method consistency ... WARNING
predictSolute:
  function(load.model, flux.or.conc, newdata, interval, level,
           lin.or.log, se.fit, se.pred, date, attach.units, agg.by,
           ...)
predictSolute.loadComp:
  function(load.model, flux.or.conc, newdata, interval, level,
           lin.or.log, se.fit, se.pred, date, attach.units, fit.reg,
           fit.resid, fit.resid.raw, agg.by, ...)
See section 'Generic functions and methods' in the 'Writing R
Extensions' manual.

Is this warning related to the one you're seeing in the vignette? Could you copy that warning here so I can see?

wdwatkins commented 7 years ago

The vignette warning is just because I made aggregateSolute internal. We'll just need to adjust the vignette to use the new predictSolute argument instead.

aappling-usgs commented 7 years ago

OK, could you create an issue for updating the vignette (unless it's easy enough to do in this PR)?

aappling-usgs commented 7 years ago

Also, when I Check locally, I'm seeing an error rather than a warning in the vignette. Is this different from what you're seeing?

Quitting from lines 120-124 (intro_to_loadflex.Rmd) 
Error: processing vignette 'intro_to_loadflex.Rmd' failed with diagnostics:
could not find function "aggregateSolute"
wdwatkins commented 7 years ago

Yeah that is the one. Apparently errors in vignettes generate warnings in R CMD CHECK

checking re-building of vignette outputs ... WARNING
Error in re-building vignettes:
wdwatkins commented 7 years ago

should probably add/change some tests so the new agg.by arguments get used.

aappling-usgs commented 7 years ago

when you said, "should probably add/change some tests", did you mean that you planned to do so for this PR? if not, could you create an issue for it?

could you also add updates or an issue for the vignette? would rather not have a broken vignette on the master branch for long.

wdwatkins commented 7 years ago

Might as well do it now.

wdwatkins commented 7 years ago

@aappling-usgs actually it might be better if you you do the vignette, you would probably have a better idea how to realistically incorporate this change into the workflow. I will make an issue.

wdwatkins commented 7 years ago

@aappling-usgs this should be good to go now, pending any additional review

aappling-usgs commented 7 years ago

cool! good work.