Skip to content
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

flesh out summarizeModel framework and loadReg2 method #165

Merged
merged 8 commits into from
Jan 26, 2017

Conversation

aappling-usgs
Copy link
Contributor

addresses #152

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.1%) to 48.008% when pulling 195ed0c on aappling-usgs:issue152 into 0a3bce1 on USGS-R:master.

@aappling-usgs
Copy link
Contributor Author

@wdwatkins - i think this PR is ready to go. could you review and merge?

Copy link
Contributor

@wdwatkins wdwatkins left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Only comment would be that maybe we should change the name of summarize.R to something a bit more specific, since the summarize model functions are elsewhere now


# summarize the coefficient estimates, standard errors, and p-values
coefs <- coef(load.model, summary=TRUE, which=loadReg.model)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

#' @export
#' @family summarizeModel
summarizeModel.loadReg2 <- function(load.model, ...) {
out <- summarizeModel(getFittedModel(load.model), ...)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

retDF <- data.frame(
RMSE = rmse(load.model, model=loadReg.model),
r.squared = loadReg.fit$RSQ, # R-square needs to change when censored values are present!! see print.loadReg.R line 131 in rloadest. is this adjusted?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We'll need to remember to update this when we deal with #144

@aappling-usgs
Copy link
Contributor Author

Good thoughts, thanks. I renamed to summarize.R to summarizeData.R - not perfect, because it's iffy whether predictions==data, but an improvement over the way-too-generic summarize, as you pointed out.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.1%) to 48.008% when pulling c3a7820 on aappling-usgs:issue152 into 0a3bce1 on USGS-R:master.

@wdwatkins
Copy link
Contributor

I guess travis and appveyor were just on break or something 🤷‍♂️ merging

@wdwatkins wdwatkins merged commit 979c7fd into DOI-USGS:master Jan 26, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants