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

Various minor updates (duplicate of broken #1149) #1151

Merged
merged 17 commits into from
Oct 31, 2016

Conversation

rykelly
Copy link

@rykelly rykelly commented Oct 28, 2016

Description

  • get.trait.data:
    • forceupdate argument defaults to FALSE
    • pft ouput dir is created if it doesn't exist (even though typically check.settings will have done that)
  • Everywhere db.open is used, I immediately schedule closing it via on.exit. This ensures the connection is closed even if the function errors out.
    • Where db.connection was wrapped in try(), check now for a result of class 'try-error' rather than 'character'
  • Added Western_* pfts to pftmapping.csv
  • get.parameter.samples moved to utils package, which is the main place it's used
  • Inadvertent but presumably correct changes to modules/uncertainty by re-roxygenizing
  • Cleanup, add overwrite args to do.conversions
  • In some but not all places, remove reliance on NOW() for finding just-added BETY entries.
    • For one thing, per Remove / deprecate use of now() from SQL insert statements #1083 we shouldn't be setting the updated/created_at timestamps in BETY manually.
    • For another, if multiple workflows are running it's entirely possible to get duplicate timestamps (at least down to the second, which is what we were using.
    • Using the psql RETURNING keyword is cleaner and safer.
    • I'd doing this everywhere in the code we're currently relying on the set/query "unique" timestamp design, but again, have currently only done it for the ones that were causing me problems.
  • Updated get.analysis.filenames to put EA/SA data files into their own directories. It gets messy otherwise when doing a MultiSettings run with hundreds of sites...

Motivation and Context

Fixing / cleaning things as they come up while setting up my multisite runs

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 documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

Ryan Kelly added 14 commits October 27, 2016 07:13
- Default to not forcing trait data update
- Create PFT directory if not already (currently done by check.settings too, but no need for get.trait.data to rely on that)
Prevents too many open connections. on.exit runs when the function exits, even on error.
This was causing bugs for me when running multiple workflows at once (not too hard to get two with the same "now" timestamp), and it also addresses PecanProject#1083
browndog = settings$browndog)
settings$run$inputs[[i]][["path"]] <- result
status.end()
if (is.null(input$path)) {
Copy link
Member

Choose a reason for hiding this comment

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

Not clear why status.check was removed from this if statement

Copy link
Author

Choose a reason for hiding this comment

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

Accident. Reverted.

@@ -23,7 +23,7 @@ ensemble.filename <- function(settings, prefix = "ensemble.samples", suffix = "R
ensemble.id <- "NOENSEMBLEID"
}

ensemble.dir <- settings$outdir
ensemble.dir <- file.path(settings$outdir, 'ensemble.samples')
Copy link
Member

Choose a reason for hiding this comment

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

This appears to put the ensemble samples in their own directory rather than in the base outdir. This will make them unaccessible via the web interface. Not sure the reason for this change

Copy link
Author

Choose a reason for hiding this comment

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

Hm. Well, the reason for this change is that it's obnoxious to have hundreds of *.samples.Rdata files in the root workflow dir if you're doing a big Multi run. But I reverted anyway, since obviously breaking the web interface is worse.

} else {
ind <- which(sapply(settings$pfts, function(x) x$name) == pft)
if (length(ind) == 0) {
## no match
logger.warn("sensitivity.filename: unmatched PFT = ", pft, " not among ",
sapply(settings$pfts, function(x) x$name))
sensitivity.dir <- file.path(settings$outdir, "pfts", pft)
sensitivity.dir <- file.path(settings$outdir, "pfts", pft, "sensitivity.samples")
Copy link
Member

Choose a reason for hiding this comment

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

same issue as above with ensemble.dir

@@ -76,14 +76,14 @@ sensitivity.filename <- function(settings,

if (is.null(pft)) {
# Goes in main output directory.
sensitivity.dir <- settings$outdir
sensitivity.dir <- file.path(settings$outdir, 'sensitivity.samples')
Copy link
Member

Choose a reason for hiding this comment

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

same issue as above with ensemble.dir

@@ -95,7 +95,7 @@ sensitivity.filename <- function(settings,
## no outdir
settings$pfts[[ind]]$outdir <- file.path(settings$outdir, "pfts", pft)
}
sensitivity.dir <- settings$pfts[[ind]]$outdir
sensitivity.dir <- file.path(settings$pfts[[ind]]$outdir, "sensitivity.samples")
Copy link
Member

Choose a reason for hiding this comment

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

same issue as above with ensemble.dir

@mdietze mdietze merged commit 0f5451e into PecanProject:master Oct 31, 2016
@rykelly rykelly deleted the various_minor_updates branch November 3, 2016 14:21
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.

2 participants