AU-BCE-EE / ABM

R model for anaerobic microbial degradation of organic matter with multiple microbial groups
1 stars 2 forks source link

Default parameter sets #35

Open sashahafner opened 1 year ago

sashahafner commented 1 year ago

@fdalby pointed out that we should include default parameter sets in the package apart from in abm.R. Users should know which set they are using and should be able to find and use older versions. Here is an option.

  1. Create a parameter object (list or vector) for each *_pars argument and include them all in the package as a data object. Would have a name and version, e.g., grp_pars2. Then users (and we!) would have an easy time using defaults for some parameters, e.g., grp_pars and mic_pars.

  2. Also create pars2 <- list(wthr_pars, ... , grp_pars,...) with all the individual par sets and include that in the pacakge as a data object. This could be the default in abm.R, included by abm(pars = pars2,...)

sashahafner commented 1 year ago

See 972345bee5bc6343ed14f8f05446cbaab594032d for an example. This could then have in abm.R:

  init_pars = list(conc_init = man_pars$conc_fresh),
  grp_pars = grp_pars2.0, # Or ABM::grp_pars2.0
  mic_pars = list(ks_SO4 = 0.00694,
                  km_urea = 0.913,
                  alpha_opt = c(urea = 60, VSd = 0.04954023),
                  alpha_T_min = c(urea = 0, VSd = 0),
                  alpha_T_opt = c(urea = 50, VSd = 50),
                  alpha_T_max = c(urea = 60, VSd = 60)),
fdalby commented 1 year ago

Yup that is a good idea. I have added v2.0 pars as Rda and used them as defaults. I added some explanation in the vignette also.

fdalby commented 1 year ago

@sashahafner I guess this works well and I am closing the issue

sashahafner commented 1 year ago

@fdalby are you happy with the mix of parameter arguments, some defined in abm.R, some as data objects? I think the current (113125ac03feacfc4ac54d38c349a97d58a8526d) approach you set up with only mng_pars defined in abm.R makes sense. All users should be using their own management inputs, except for demos.

How about this though?

evap_pars = list(evap = 0.5 * et(temp_C = wthr_pars2.0$temp_air_C, pres_kpa = wthr_pars2.0$pres_kpa, rs = wthr_pars2.0$rs)),                # mm/d

Could we keep it as wthr_pars$temp_air_C etc? That way if users use other wthr_pars the evap info will be pulled from that.

fdalby commented 1 year ago

Oops you right should be just wthr_pars. Otherwise I also agree that the user need to actively think about mng_pars arguments and we should not give a pars vx.x set for that.

fdalby commented 1 year ago

Could we keep it as wthr_pars$temp_air_C etc? That way if users use other wthr_pars the evap info will be pulled from that.

now fixed

sashahafner commented 11 months ago

Could we keep it as wthr_pars$temp_air_C etc? That way if users use other wthr_pars the evap info will be pulled from that.

now fixed

Great

sashahafner commented 11 months ago

@fdalby I noticed the scale inputs in mng_pars. Does that make sense? I guess the idea is management could affect these things. . .

sashahafner commented 11 months ago

@fdalby should we (can we) have a (some) parsvXX objects made up of the parts (man, grp, etc.) for use with the pars argument? Do you use that argument at all?

fdalby commented 11 months ago

@fdalby should we (can we) have a (some) parsvXX objects made up of the parts (man, grp, etc.) for use with the pars argument? Do you use that argument at all?

@sashahafner I have never used the pars argument - not even sure how it works :)? Question is if we need to do more work on that if we are not using it. It is ok for me if you want to.

fdalby commented 11 months ago

@fdalby I noticed the scale inputs in mng_pars. Does that make sense? I guess the idea is management could affect these things. . .

@sashahafner . It might not make a lot of sense. Perhaps we should just remove it. Even putting it in grp_pars would still not make sense, because e.g. scale.alpha_opt, is not in grp_pars. I can remove if you agree.

sashahafner commented 11 months ago

Let's leave pars alone then. Doesn't make sense to try to combine them in pars objects because the management parameters (others too?) would be missing.

Actually why not remove it then? I can't think of a case where we really need it.

Done in 25f59f163e3d78e13f0500e58c1d3219e91a6f42