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.

Sign up for GitHub

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

Jump to bottom

Style cleanup for `db/R/*.R` scripts  #1091

Closed

bpbond wants to merge 24 commits into PecanProject:develop from bpbond:style-update-db

Contributor

bpbond commented Oct 17, 2016

Ran through formatR::tidy_dir with arrow=TRUE and indent=2; made sure all if blocks have braces. Tried to make sure formatR didn’t make things too jaggy. Substituted seq_len, seq_along, and paste0, and file.path as necessary. Made sure all functions end with an explicit return.


          Style cleanup for db/R/*.R scripts

3e7a079

Ran through formatR::tidy_dir with arrow=TRUE and indent=2; made sure
all if blocks have braces. Tried to make sure formatR didn’t make
things too jaggy. Substituted `seq_len`, `seq_along`, and `paste0`, and
`file.path` as necessary. Made sure all functions end with an explicit
`return`.

Member

mdietze commented Oct 17, 2016

Throwing similar errors to #1042

Contributor Author

bpbond commented Oct 17, 2016

Hmm. The test code that's failing (test.contents_sanity.R) is checking db/R/utils/db.query.R, specifically dbquery(), which is I guess returning NULL to every query. I don't see any change to this function that would cause this...going to trigger another build, just in case, I don't know, maybe psql-pecan.bu.edu was down?

bpbond added 2 commits

October 17, 2016 02:22


          Paste to paste0 - trigger another build

f9f06ed


          Merge remote-tracking branch 'PecanProject/master' into style-update-db

bpbond commented

View reviewed changes

db/R/dbfiles.R Outdated

                 }
                 if (is.null(inputid)) {
-                  invisible(data.frame())
+                  return(invisible(data.frame()))

Contributor Author

bpbond Oct 17, 2016

Significant change: before an invisible data.frame was being created but not used, and execution continued. Now it's returned (as I thought was the intention).

Member

robkooper Oct 17, 2016

not sure the return is needed. I thought R would return the last computed value, in this case either of the invisible statements in the if statement.

Contributor Author

bpbond Oct 17, 2016

@robkooper agreed, it's not; but @mdietze and others have expressed preference for explicit returns, so I've started doing that.

Member

robkooper Oct 18, 2016

I sort of agree with them, I had always thought that invisible would return as well, until I realized some of my code does not work. So I do think having an explicit return(invisible()) is a good thing.

Member

robkooper Oct 18, 2016

@tonygardella can we make sure we add to the styleguide section that we are using explicit returns.

Contributor

tonygardella Oct 18, 2016

@robkooper Got it!

mdietze requested changes

View reviewed changes

Member

mdietze left a comment

Tried to tag places where Travis was unhappy. get.id's colname vs colnames definitely needs fixing. In terms of the failed tests, they all fail due to "expired PostgreSQLConnection" starting with Error: citations table exists and has >= 1 columns. This makes me think that something's closing the connection prematurely

db/R/utils.R

    
              ##' }

              get.id <- function(table, colnames, values, con, create=FALSE, dates=FALSE){

              get.id <- function(table, colnames, values, con, create = FALSE, dates = FALSE) {

Member

mdietze Oct 17, 2016

Second argument here is colnames (plural). In some of the calls to this function we're using colname, others are using colnames, and others aren't passing by name. Need to make this consistent. colname is clearly wrong.

db/R/query.trait.data.R

               ##' @author Carl Davidson, Ryan Kelly
               ##' @export
               ##--------------------------------------------------------------------------------------------------#
-              append.covariate<-function(data, column.name, covariates.data){

Member

mdietze Oct 17, 2016

Travis says "Must have a \description"

db/R/query.trait.data.R

@@ @@ -172,33 +169,32 @@ query.covariates<-function(trait.ids, con = NULL, ...){ @@
               ##' @param new.temp the reference temperature for the scaled traits. Curerntly 25 degC
               ##' @param missing.temp the temperature assumed for traits with no covariate found. Curerntly 25 degC
               ##' @author Carl Davidson, David LeBauer, Ryan Kelly
-              arrhenius.scaling.traits <- function(data, covariates, temp.covariates, new.temp=25, missing.temp=25){
+              arrhenius.scaling.traits <- function(data, covariates, temp.covariates, new.temp = 25,
+                                                   missing.temp = 25) {

Member

mdietze Oct 17, 2016

Travis says "Must have a \description"

db/R/query.trait.data.R

-                  data <- append.covariate(data, 'canopy_layer',
-                                           covariates[covariates$name == 'canopy_layer',])
-                  data <-  data[data$canopy_layer >= 0.66 | is.na(data$canopy_layer),]
+              filter.sunleaf.traits <- function(data, covariates) {

Member

mdietze Oct 17, 2016

Travis says "Must have a \description"

db/R/query.trait.data.R

-                covariate.query <- paste("select covariates.trait_id, covariates.level,variables.name",
-                                         "from covariates left join variables on variables.id = covariates.variable_id",
-                                         "where trait_id in (",vecpaste(trait.ids),")")
+              query.covariates <- function(trait.ids, con = NULL, ...) {

Member

mdietze Oct 17, 2016

Travis says "Must have a \description"

db/R/query.file.path.R

-                path <- file.path(dbfile$file_path,dbfile$file_name)
-                cmd <- paste0("file.exists( '",path,"')")
-                remote.execute.R(cmd,machine.host,verbose=TRUE)
+              query.file.path <- function(input.id, host_name, con) {

Member

mdietze Oct 17, 2016

Travis says "Must have a \description"

db/R/query.format.vars.R Outdated

@@ @@ -6,107 +6,113 @@ @@
               ##'
               ##' @author Betsy Cowdery , Ankur Desai
               ##'
-              query.format.vars <- function(input.id,con,format.id){
+              query.format.vars <- function(input.id, con, format.id) {

Member

mdietze Oct 17, 2016

Travis says "Must have a \description"

db/R/query.site.R

@@ @@ -5,14 +5,17 @@ @@
               ##' @export
               ##'
               ##' @author Betsy Cowdery
-              ##'
-              query.site <- function(site.id,con){
+              query.site <- function(site.id, con) {

Member

mdietze Oct 17, 2016

Travis says "Must have a \description"

db/R/query.trait.data.R

@@ @@ -236,25 +231,23 @@ filter.sunleaf.traits <- function(data, covariates){ @@
               ##' @seealso used with \code{\link{jagify}};
               ##' @export
               rename.jags.columns <- function(data) {

Member

mdietze Oct 17, 2016

Travis says "Must have a \description"

bpbond commented

View reviewed changes

db/R/dbfiles.R

                 # find appropriate pft
                 pftid <- get.id("pfts", "name", pft, con)
                 if (is.null(pftid)) {
-                  invisible(data.frame())
+                  return(invisible(data.frame()))

Contributor Author

bpbond Oct 17, 2016

Significant change: before an invisible data.frame was being created but not used, and execution continued. Now it's returned (as I thought was the intention).

Member

robkooper Oct 17, 2016

nice catch

bpbond commented

View reviewed changes

db/R/dbfiles.R

                 if (is.null(posteriorid)) {
-                  invisible(data.frame())
+                  return(invisible(data.frame()))

Contributor Author

bpbond Oct 17, 2016

Same.

bpbond commented

View reviewed changes

db/R/dbfiles.R Outdated

                 if (is.null(hostid)) {
-                  invisible(data.frame())
+                  return(invisible(data.frame()))

Contributor Author

bpbond Oct 17, 2016

And again.

bpbond commented

View reviewed changes

db/R/dbfiles.R Outdated

    
                  logger.warn("multiple files found for", id, "returned; using the first one found")

                  invisible(file.path(files[1, 'file_path'], files[1, 'file_name']))

                  return(invisible(file.path(files[1, "file_path"], files[1, "file_name"])))

Contributor Author

bpbond Oct 17, 2016

Also.

bpbond commented

View reviewed changes

db/R/dbfiles.R

                 if (is.null(hostid)) {
-                  invisible(NA)
+                  return(invisible(NA))

Contributor Author

bpbond Oct 17, 2016

Last one. Significant change: before an invisible data.frame was being created but not used, and execution continued. Now it's returned (as I thought was the intention).


          Formatting was better in the original

a3bb2f7

Contributor Author

bpbond commented Oct 17, 2016

Thanks, Mike. I can add placeholder \description fields and correct the colname vs. colnames use. But...I'm still confused why these weren't causing errors before. None of the changes in this PR relate to these.


          Correct colname usage and add @description tags

db7c868

Member

mdietze commented Oct 17, 2016

@bpbond conflict on get.trait.data.R


          Merge remote-tracking branch 'PecanProject/master' into style-update-db

e97e18f

# Conflicts:
#	modules/data.atmosphere/R/load.cfmet.R
#	modules/data.atmosphere/R/met2CF.NARR.R
#	modules/data.atmosphere/R/temporal.downscaling.R

Member

mdietze commented Oct 19, 2016

Just tried rebuilding this PR in light of all the other changes. Still fails but gets much further (SIPNET test case)

Error in postgresqlQuickSQL(conn, statement, ...) : 
  expired PostgreSQLConnection
Calls: runModule.get.trait.data ... db.query -> dbGetQuery -> dbGetQuery -> postgresqlQuickSQL

@robkooper or others might know better than me, but this error suggests that some function is closing the db connection that shouldn't, and thus downstream code is getting passed a closed connection.

Member

mdietze commented Oct 19, 2016

@bpbond might be worth a try to just resolve conflicts and update to see if changes elsewhere have resolved some of the issues we're seeing here.

I'll just be here slouching and watching DOE's ergonomics videos while I wait for that (HR finally caught me!)

Contributor Author

bpbond commented Oct 19, 2016

ergonomics videos

You have fun with that!


          Merge remote-tracking branch 'PecanProject/master' into style-update-db

ed720e7

Conflict resolution. I hope.

mdietze approved these changes

View reviewed changes

Contributor Author

bpbond commented Oct 20, 2016 •

edited

The packages all install, but a test fails (same as above):

TEST travis.sipnet.xml FAILED (ERROR IN STATUS)
----------------------------------------------------------------------
TRAIT   2016-10-19 22:11:37 2016-10-19 22:11:37 ERROR   
Loading required package: PEcAn.DB
...
Loading required package: MCMCpack
##
## Markov Chain Monte Carlo Package (MCMCpack)
## Copyright (C) 2003-2016 Andrew D. Martin, Kevin M. Quinn, and Jong Hee Park
##
## Support provided by the U.S. National Science Foundation
## (Grants SES-0350646 and SES-0350613)
##
2016-10-19 22:11:36 INFO   [read.settings] : 
   Loading --settings= travis.sipnet.xml 
2016-10-19 22:11:37 INFO   [fn] : 
   settings$run$dbfiles is deprecated. uwe settings$database$dbfiles 
   instead 
2016-10-19 22:11:37 INFO   [fn] : 
   settings$run$host is deprecated. uwe settings$host instead 
Loading required package: RPostgreSQL
2016-10-19 22:11:37 INFO   [check.database] : 
   Successfully connected to database : PostgreSQL bety bety localhost bety 
   FALSE 
2016-10-19 22:11:37 WARN   [check.database.settings] : 
   Will not write runs/configurations to database. 
2016-10-19 22:11:37 WARN   [check.bety.version] : 
   Last migration 20160711231257 is more recent than expected 
   20141009160121. This could result in PEcAn not working as expected. 
Warning: hms, hm and ms usage is deprecated, please use HMS, HM or MS instead. Deprecated in version '1.5.6'.
Warning: hms, hm and ms usage is deprecated, please use HMS, HM or MS instead. Deprecated in version '1.5.6'.
2016-10-19 22:11:37 INFO   [fn] : 
   Setting ensemble size to 1. 
2016-10-19 22:11:37 INFO   [fn] : 
   No start date passed to ensemble - using the run date ( 2002 ). 
2016-10-19 22:11:37 INFO   [fn] : 
   No end date passed to ensemble - using the run date ( 2005 ). 
2016-10-19 22:11:37 INFO   [fn] : 
   No start date passed to sensitivity.analysis - using the run date ( 2002 
   ). 
2016-10-19 22:11:37 INFO   [fn] : 
   No end date passed to sensitivity.analysis - using the run date ( 2005 
   ). 
2016-10-19 22:11:37 INFO   [fn] : 
   Setting site name to Niwot Ridge Forest/LTER NWT1 (US-NR1) 
2016-10-19 22:11:37 INFO   [fn] : 
   Setting site lat to 40.0329 
2016-10-19 22:11:37 INFO   [fn] : 
   Setting site lon to -105.546 
2016-10-19 22:11:37 INFO   [check.model.settings] : 
   Setting model id to 10 
2016-10-19 22:11:37 INFO   [check.model.settings] : 
   Setting model revision to unk 
2016-10-19 22:11:37 INFO   [check.model.settings] : 
   Option to delete raw model output not set or not logical. Will keep all 
   model output. 
2016-10-19 22:11:37 WARN   [dbfile.file] : 
   no files found for 10 in database 
2016-10-19 22:11:37 WARN   [check.settings] : 
   settings$database$dbfiles pathname pecan/dbfiles is invalid 

   placing it in the home directory /home/travis 
2016-10-19 22:11:37 INFO   [check.workflow.settings] : 
   output folder = /home/travis/build/PecanProject/pecan/tests/pecan 
2016-10-19 22:11:37 INFO   [check.settings] : 
   Storing pft temperate.coniferous in 
   /home/travis/build/PecanProject/pecan/tests/pecan/pft/temperate.coniferous 
Warning in file.remove(file.path(settings$outdir, "STATUS")) :
  cannot remove file '/home/travis/build/PecanProject/pecan/tests/pecan/STATUS', reason 'No such file or directory'
[1] FALSE
Error in postgresqlQuickSQL(conn, statement, ...) : 
  expired PostgreSQLConnection
Calls: runModule.get.trait.data ... db.query -> dbGetQuery -> dbGetQuery -> postgresqlQuickSQL

bpbond mentioned this pull request

Make sure functions end with explicit return()s #1134

Merged

8 tasks


          Merge branch 'master' into style-update-db

9b05f27

Member

dlebauer commented Mar 31, 2017

@bpbond I was looking through old PRs. I was going to ask if you wanted to resolve the conflicts, then took a look, saw GitHub has a new way to resolve conflicts in the browser and got started. It wouldn't let me save a draft so I went through and tried to resolve everything.

This package is pretty complex, what with pasting SQL queries together and all, and I have no desire to debug any errors that may arise so I am still uncertain if it is worth merging this. (time will be better spent porting to dplyr ... ). So if anything non-trivial pops up I would suggest we close the PR and either re-format the package from master branch or move on and never look back.

In any case, thanks again for doing so much work into making PEcAn's formatting more consistent.

Contributor Author

bpbond commented Apr 3, 2017

if anything non-trivial pops up I would suggest we close the PR
Agreed!

Sorry I let this drop. If there are other architectural or science things you guys would like me to consider/look at, feel free to ping me if useful.

Ben

dlebauer and others added 3 commits

April 5, 2017 23:13


          Merge branch 'master' into style-update-db

b76f872


          cleaning up using git page

ce26a2e


          delete quick and easy link

fc923bd

tonygardella and others added 13 commits

April 14, 2017 15:42


          update changelog

1b9ea85


          Merge branch 'master' into style-update-db

bba4cff


          fixed a figure and added link to Demo1

04a5d0f


          table for vignettes and demos

3fd32bb


          add links in the top and fix workflow sections

c27a04c


          typo

ac9a865


          more typos

374b2bb


          fix table of demos and vignettes

4baddea


          get rid of table centering

ff99d9e


          Merge pull request #1352 from tonygardella/docs

e2d2597

Cleaning up and updating "Using Git" page


          Revert "Cleaning up and updating "Using Git" page"

7cc2743


          Merge pull request #1360 from PecanProject/revert-1352-docs

f25c810

Revert "Cleaning up and updating "Using Git" page"


          Merge branch 'master' into style-update-db

53e94bf

tonygardella changed the base branch from master to develop

April 21, 2017 12:33

Member

dlebauer commented Jul 8, 2017

At this point it would be easier to re-run the clean-up described in the original PR comment than to resolve remaining conflicts and hope that these and previously resolved conflicts are correct.

mdietze mentioned this pull request

reapply DB clean-up #1520

Closed

mdietze closed this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment