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

allow non-factor statnames in utils::transformstats #2545

Merged
merged 8 commits into from Mar 26, 2020

Conversation

infotroph
Copy link
Member

@infotroph infotroph commented Mar 6, 2020

Description

PEcAn.utils::transformstats currently assumes the statname column if its input is a factor. This patch allows it to be character instead, if desired. Output keeps the same class it had on input.

Edit: while I was here, also cleaned up code formatting and fixed logic in the untransformed stats warning.

Motivation and Context

Noticed now because the development version of R no longer coerces character vectors in data.frame to factor, so our unit test started failing. Good catch, unit test!

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.
  • 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.

@infotroph infotroph requested a review from dlebauer March 6, 2020 21:16
@infotroph
Copy link
Member Author

@mdietze @dlebauer I think this is ready to merge -- previous Travis failures are hopefully now resolved.

@dlebauer
Copy link
Member

I still don't understand travis well enough to know what to do with the travis failure - the log says "build exited with status 1" indicating success, but then travis says the build fails https://travis-ci.org/github/PecanProject/pecan/jobs/664465417?utm_medium=notification&utm_source=github_status

@infotroph
Copy link
Member Author

@dlebauer the statuses are from bash, so anything not 0 is a failure.

Looks like this is yet another unrelated dependency failure, this time in the DB tests: apparently PEcAn.DB::build_insert_query is now returning doubled quotes because of something that changed between glue 1.3.1 and 1.3.2

@ashiklom
Copy link
Member

As I mentioned on Slack, I think the easiest fix here is to replace the glue_sql statement in base/db/R/insert_table.R::build_insert_query with the following:

insert_string <- paste(insert_list, collapse = ",")
result <- glue::glue_sql(
  "INSERT INTO {`table`} ({`colnames(values)`*}) VALUES ",
  .con = .con
)
DBI::SQL(paste0(result, "(", insert_string, ")"))

It feel like a less-robust solution because it replaces supposedly robust backend-dependent SQL quoting with hard-coded quoting...but I think it should work with the SQLite backend in the test.

Also as I mentioned on Slack, the ideal solution here is to simplify the code in that script to just use prepared statements for everything, which should be faster, safer, and easier to implement.

@mdietze mdietze merged commit 465a566 into PecanProject:develop Mar 26, 2020
@infotroph infotroph deleted the transformstats-factors branch April 14, 2020 20:42
@robkooper robkooper added this to the 1.8.0 milestone Apr 14, 2020
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.

None yet

5 participants