gadget-framework / gadget3

TMB-based gadget implemtation
GNU General Public License v2.0
8 stars 6 forks source link

g3_init_guess migration / replacement #124

Closed lentinj closed 6 months ago

lentinj commented 9 months ago

At some point we need an "official" equivalent in gadget3. This would be a good chance to rethink the interface a bit. Some thoughts:-

lentinj commented 8 months ago

@bthe Any thoughts on the above? Anything missing?

bthe commented 8 months ago

No, this look pretty good.

lentinj commented 7 months ago

I think the automagic "Append a \\.[0-9] to the regex if it will find something" is an anti-feature and should be removed:

https://github.com/gadget-framework/gadgetutils/blob/1cfcf2078824b1959df0d558bb8f4e248ab6c10a/R/parameter_template_utils.R#L24-L30

Instead you can just do .# when you know these are time-varying. This will be a bit annoying if the variables become time varying but would be less surprising.

The "Auto-exponentiate" option is worth keeping though, since setting it up yourself will be annoying, and break all the lower/upper conveniences.

lentinj commented 7 months ago

@bthe @willbutler42 What do you make of this? https://github.com/gadget-framework/gadget3/commit/5c0aef7951117aae105941e8ecc2284b6f554aee#diff-3ab8b8e45cb9c55c14016dc20ec32a4d71467674b522b171c6f5d6e2b32bae8eR96

I think covers everything we need, but if you see anything that won't work let me know.

willbutler42 commented 7 months ago

I am just testing this now. A definite improvement, though having trouble with the auto_exponentiate part here https://github.com/gadget-framework/gadget3/blob/issue-124-g3_init_val/R/init_val.R#L70C35-L70C35, think either the log function needs to be applied over list elements , or the list needs to be unlisted?

Also, we may need to re-think the default names for the maturity and selectivity functions: name_spec_matched('*.alpha', c('fleetname.alpha', 'bling.mat.alpha'))

lentinj commented 7 months ago

though having trouble with the auto_exponentiate part here

Ooops, forgot about the value being a list, will look into it, thanks!

Also, we may need to re-think the default names for the maturity and selectivity functions

I've got a patch that hasn't made it here to support |, so you could do *.imm|mat.alpha. This would make this a bit better but I agree, the names are unnecessarily vague.

lentinj commented 7 months ago

Should have improved matters now, and you can do *.imm|mat.alpha too.