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

Updating the LandTrendr.AGB.R functions #2909

Draft
wants to merge 4 commits into
base: develop
Choose a base branch
from

Conversation

serbinsh
Copy link
Member

@serbinsh serbinsh commented Feb 15, 2022

Updating the LandTrendr biomass functions to work with the new v2 dataset and write file locations to betydb

Description

Confirming functions work with v1 and v2. Other fixes. Updating file tracking to use BETYdb

Motivation and Context

Write files to BETYdb & bug fixes

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.
  • My name is in the list of .zenodo.json
  • 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.

@serbinsh serbinsh marked this pull request as draft February 15, 2022 22:32
download_urls_final <- download_urls
# ------ new way using the database
if (product_version == "v1") {
median_input_id <- 2000000234
Copy link
Member

Choose a reason for hiding this comment

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

Not sure why we're hard coding input ID's

##' @export
##' @author Shawn Serbin
##'
download.LandTrendr.AGB <- function(outdir, target_dataset = "biomass", product_dates = NULL,
product_version = "v1", con = NULL, run_parallel = TRUE,
ncores = NULL, overwrite = FALSE) {
product_version = "v1", run_parallel = TRUE,
Copy link
Member

Choose a reason for hiding this comment

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

Not seeing how you specify what locations you want to download

@@ -183,6 +264,8 @@ download.LandTrendr.AGB <- function(outdir, target_dataset = "biomass", product_
results$formatname[i] <- out_formats[i]
}

suppressWarnings(PEcAn.DB::db.close(con, showWarnings = FALSE)) # why isnt TRUE invisible?
Copy link
Member

Choose a reason for hiding this comment

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

Please remove this line. Functions should not close db connections that they were passed as arguments (and thus that they didn't open themselves) because they don't know if that connection will be needed for a later function.

data_dir = NULL, product_dates = NULL, output_file = NULL,
...) {

## get coordinates and provide spatial info
## get coordinates and provide spatial info - should harmonize what packages we use in all data.remote functions
site_info <- site_info
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 what this line is trying to achieve

data_dir = NULL, product_dates = NULL, output_file = NULL,
...) {

## get coordinates and provide spatial info
## get coordinates and provide spatial info - should harmonize what packages we use in all data.remote functions
site_info <- site_info
site_coords <- data.frame(site_info$lon, site_info$lat)
Copy link
Member

Choose a reason for hiding this comment

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

This line will crash if site_info is NULL. To me, it seems like you'd want to revert your change above that allows site_info to be NULL, but if you decide to keep it you need to add a lot of error checking for what to do if the site is null.

@mdietze
Copy link
Member

mdietze commented Apr 26, 2023

@serbinsh just a reminder about this PR. Not pressing, but would be good to wrap this up

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.

4 participants