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

Regeneration delays ('delays' object) update #25

Merged
merged 1 commit into from
Jan 7, 2025

Conversation

suz-estella
Copy link
Contributor

Suggesting this update to CBM_core that would simplify the job of specifying regeneration delays for data prep modules. @cboisvenue let's chat about this! I've added a couple specific questions at the bottom.

Currently, CBM_core always requires the delays input object, which specifies post disturbance regeneration delays for each pixel group. This has to be the same length as nrow(level3DT) even if all delays are set to the same number (e.g. 0). There is no default.

Instead of always requiring the delays input object, this instead does 2 new things:

  1. Adds a regenDelay parameter that allows for a default regeneration delay unless otherwise specified. This defaults to 0 but could be changed by the user.
  2. Allows the user to optionally include a regenDelay column with the level3DT table so that delays can be specified individually for each pixel group. Where there are NAs, the regenDelay parameter is used.

This makes it easier to use CBM_core as the user can do any of:

  1. Not specify delays at all and use the default of 0 years for all pixel groups (via the regenDelay parameter).
  2. Easily specify another single number to use for all pixel groups (via the regenDelay parameter).
  3. If desired: Provide specific delays for each pixel group (via the regenDelay column of level3DT), but with the benefit of having the level3DT table structure define the required length of the vector. This also allows for the delays to stay closely linked with their pixel group ID.

It seems that CBM_dataPrep_SK is creating the delays object only to set them all to 0 if not otherwise specified. Since there's no other special "SK-specific" behavior here I would say it would be best to leave this job to CBM_core. If this is merged, this PR for CBM_dataPrep_SK can also be merged: PredictiveEcology/CBM_dataPrep_SK#13

Questions:

  1. Any reason not to allow a default for this parameter? Could this be allowable if there is a message to the user that the default delay is being used?
  2. Any reason not to allow for an optional extra column in the level3DT table? This could be scrapped to instead keep the delays object, but just as an optional input. It seems ideal to me (my newbie eyes) to lean towards having pixel-group specific attributes all in one place, but I see that there are other similar cases (e.g. historicDMtype is also a vector that needs to be the length of nrow(level3DT)), so it may make sense to take a step back and think about how all of these should be best handled. My concern is that having pixel group attributes outside of the level3DT table without a key to link them back means that there may be opportunity for the rows & vectors to get reordered and misattributed somewhere down the line.

…th the option to provide a 'regenDelay' column in the 'level3DT' table.
@cboisvenue cboisvenue merged commit 7c9debd into PredictiveEcology:development Jan 7, 2025
1 check passed
@cboisvenue
Copy link
Contributor

@suz-estella we can do the same with historicDMtype as in Canada it will always have a value of 1, unless the user wants to do some exploring.

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