gjearevoll / BioDivMapping

A pipeline dedicated to analysing and visualising the biodiversity of different taxa in Norway
GNU General Public License v3.0
5 stars 3 forks source link

updates #111

Closed Peprah94 closed 5 months ago

Peprah94 commented 7 months ago

Why have changes been made?

I have made changes to three files. One has to do with selection of environmental covariates and the other has to do with the selection of focal taxon without going through the excel file. The last change has to setting the encoder for the api when working with windows device.

What changes have been made?

Peprah94 commented 7 months ago

The branch used the latest version of the main branch.

RRTogunov commented 7 months ago

a few changes required:

  1. masterScript.R defines level, region, crs, and res on lines 26-32 and on 72-76 (same with scheduledDownload, waitForGbif, focalTaxonIndex, and myMesh). Also, definition of mesh on L95 in masterScript, has been integrated into L84 of environmentalImport.R. I think these are carryovers from previous version of masterScript.R
  2. a) also, in your edit to pipeline/import/environmentalImport.R, should "rep(TRUE, length(focalTaxonIndex))" be "rep(TRUE, length(environmentalCovariates))"?
  3. b) (alternative to 2a), in both environmentalImport.R and taxaImport.R, I don't think you need to specify number of FALSE & TRUE; ie. you should be able to just have:
    focalTaxon$include <- FALSE
    focalTaxon$include[focalTaxon$taxa %in% focalTaxonIndex] <- TRUE

    or even more simply: focalTaxon$include <- focalTaxon$taxa %in% focalTaxonIndex

  4. double-check this, I think your PR, wouldn't selectedParameters always be identical to environmentalCovariates (assuming user doesn't specify covariates that don't exist in parameters$parameters). If I'm not mistaken, couldn't selectedParameters be specified directly in masterScript? specifically, L99 in masterScript.R would become:
    selectedParameters <- c("aspect", 
                             "elevation", 
                             "precipitation", 
                             "temperature")

    and L56-61 in environmentalImport.R would become:

    if(!exists("selectedParameters ")){
    selectedParameters <- parameters$parameters[parameters$selected]
    }

    If you make these 3 changes and @samaperrin is happy, it should be good to merge.

Two general comments:

  1. I agree, it should be easier to adjust things in the pipeline than is currently done; I just made an issue detailing thoughts and possible solutions.
  2. Also agree that big-picture, it would be good to generalise specification of pipeline arguments within R and not necessarily rely on external CSVs and enable working exclusively in R. For example, allow users to provide either a file path (what is currently implemented) or formatted data.frame (more common workflow in other packages) for things like focalSpecies and environmental import. We should make this into an issue and discuss with @oharar and others about what we want the workflow to look like for users beyond this specific contract.
samaperrin commented 7 months ago

Yeah you've made some updates to an outdated version of the repo, some of these are made redundant by @RRTogunov 's recent updates. It might be worth checking out the latest version and then seeing what still needs to be updated.

Peprah94 commented 7 months ago

I copied the main into my branch before running the code to make these changes. When was the main branch updated?

Kwaku.

On Wed, 7 Feb 2024 at 22:54, Sam Perrin @.***> wrote:

Yeah you've made some updates to an outdated version of the repo, some of these are made redundant by @RRTogunov https://github.com/RRTogunov 's recent updates. It might be worth checking out the latest version and then seeing what still needs to be updated.

— Reply to this email directly, view it on GitHub https://github.com/gjearevoll/BioDivMapping/pull/111#issuecomment-1933001019, or unsubscribe https://github.com/notifications/unsubscribe-auth/AHQDCGUDLZVRUKKZVRTSH3DYSPZYBAVCNFSM6AAAAABC4H52SWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSMZTGAYDCMBRHE . You are receiving this because you authored the thread.Message ID: @.***>