Fertilization scripts using magic scenarios sheet - #4050
Conversation
| #replace the Inventory input source in 01-build-parcel-design.R | ||
| #need to switch the input from historical matched LandIQ + MSLSP parquet to ur county_landiq_predictions_with_phenology files | ||
|
|
||
| setwd("/projectnb/dietzelab/ananyak") |
There was a problem hiding this comment.
per comment in your other PR, let's move away from setwd
| library(data.table) | ||
|
|
||
| #scenario = either bau or nbs targets | ||
| config = list(seed = 42, scenario = "NBS_Targets",years = 2024:2045, |
There was a problem hiding this comment.
I'd restructure this so that each variable is on its own line and has a comment documenting what the variable is
|
|
||
| ##-----script 1----- | ||
| #logger replacements for pecan package parts | ||
| logger_info = function(...) message(paste0(...)) |
There was a problem hiding this comment.
Why are you creating logger replacements rather than just using the pecan logger functions?
| plant = plant |> | ||
| dplyr::filter(.data$parcel_id %in% picked) | ||
|
|
||
| logger_info("Sampled ", length(picked), " parcels using n_parcels = ", n_parcels)} |
There was a problem hiding this comment.
Style: don't put the closing } on the same line as the last line of code
|
|
||
| logger_info("Wrote staging file: ", staging_file) | ||
|
|
||
| ##-----script 2----- |
There was a problem hiding this comment.
- not following why things are labeled "script 1" "script 2" etc it they're all in the same script
- script 1, 2, etc aren't informative names. If these are distinct steps in the workflow then there should be better documentation about what each step is
|
|
||
| ##-----script 2----- | ||
|
|
||
| logger_info = function(...) message(paste0(...)) |
There was a problem hiding this comment.
redefining functions you've already defined. Also, as noted previously, should use the pecan loggers rather than redefine them
| design_file = file.path(staging_dir, "_staging_01_design.rds") | ||
|
|
||
| if (!file.exists(design_file)) { | ||
| logger_severe("Stage 01 output not found: ", design_file,". Run fertilization_projection_design.R first.")} |
There was a problem hiding this comment.
references a script that's not in this PR and references an object that was generated a few lines back, not by the referenced script, which means this logger message is misleading
|
|
||
| logger_info("Wrote staging file: ", staging_file) | ||
|
|
||
| ##-----script 3----- |
There was a problem hiding this comment.
Looking at PR 4003, it looks like you've manually moved a lot of existing code around into new locations. I'd STRONGLY recommend against this, as it creates a lot of code duplication, which (a) makes the code harder to maintain and (b) makes is confusing to users as to which version of code they should be using. From a reviewer's perspective, it also makes it very hard to know what's old code and what are new modifications. Is there any reason to not just use the older code as written? Are there ways to modify that code to support both the inventory and projection workflows?
| @@ -0,0 +1,521 @@ | |||
| # modified ncc-statewide projection workflow from PR #4003 | |||
There was a problem hiding this comment.
At this point in time I'm not reviewing this file line-by-line as it appears to have all the same issues as fertilization.R that need to be fixed first.
2 participants
The file 'ncc.R' builds off the scripts in PR 4003 to generate the organic amendment events, but now according to the magic scenarios sheet. It splits the parquet outputs into two separate folders based on the scenario:
/projectnb/dietzelab/ananyak/ncc/BAU_Targets /projectnb/dietzelab/ananyak/ncc/NBS_Targets