Make NCC compost timing crop specific and align both event workflows on the latest version monitoring - #4079
Conversation
| phen_dir: !expr path.expand(Sys.getenv("CCMMF_PHEN_DIR", "/projectnb/dietzelab/ccmmf/management/phenology/matched_landiq_mslsp_v4.1.2/gapfill_dates")) | ||
| phen_glob: !expr Sys.getenv("CCMMF_PHEN_GLOB", "assigned_year=*_gapfilled.parquet") |
There was a problem hiding this comment.
@hdpriest-ui these defaults are handy for development, but should we move toward a workflow where we always explicitly set these environment variables? And if so, is now the time to do so?
There was a problem hiding this comment.
I would definitely encourage us to move away from using absolute hardcoded paths in these configurations.
If i were to pull this, I definitely won't have these paths available to these R scripts. In addition, I would caution against using !expr in yaml files, as R is not the only language we are using for this project, and not the only language that may need to parse this yaml file.
for example, when i use one of those lines as a input to the linux yq tool, i get something like:
path.expand(Sys.getenv("CCMMF_PHEN_DIR", "testPath"))
as the value returned from the yaml read on that key.
the same will happen if a person reads this using the python implementation of yaml parsing from posit, and if they use the standard python yaml library, it will throw a ConstructorError.
the lines above are documented in the readme as config entries which have "overrides", but for anyone not on the BU server, these are actually required input configuration entries. Because they are interpreted within the config itself, there is also no way to reconstruct the actual run-time configuration passed to an execution of these scripts, because the critical variables are in the calling environment rather than on disk.
|
The points above are valid and apply across CCMMF as we move toward more mature code. For this PR's scope though, I think it looks good. I'd like to merge this now and tackle the broader items in follow-up work. |
mdietze
left a comment
mdietze
left a comment
There was a problem hiding this comment.
FWIW I agree with @hdpriest-ui point, which I think applies across a lot of MAGIC scripts
Because they are interpreted within the config itself, there is also no way to reconstruct the actual run-time configuration passed to an execution of these scripts, because the critical variables are in the calling environment rather than on disk.
That said, I also agree with @sarahkanee to pull this now and refine in a future PR
d38bea7
5 participants
Description
NCC timed compost by a binary annual/perennial split, with the offset always subtracted from a single green-up anchor. This makes the rule per PFT and allows an event to fall after its anchor.
Both workflows now read the gap-filled LandIQ to MSLSP match, which carries every phenology transition keyed
(parcel_id, year, season). Crop cycles join on the season key instead of by rank.Fertilization changes are included for input consistency, on adopting latest version of monitoring. Its timing is unchanged, date is still anchor itself, and
mslsp_50PCGIis transitionleafondaywas derived from.Changes
config.yml. Row and rice onmslsp_OGI, woody onmslsp_50PCGI, hay onmslsp_OGDLandIQ_cropCode_lookup_table.csv, the table the monitoring products use. Alfalfa and pasture move fromrowtohay,G6/G7fromhaytorowcheck-result.Rasserts each event falls inside its PFT's window. The year bound now allows a year on each side, since signed offsets can leave the anchor's year in either directionv2.0Motivation and Context
Review Time Estimate
Types of changes
Checklist: