search

gadget-framework / gadget3

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

time lookup formulas working for pseudoyears #111

Closed willbutler42 closed 1 year ago

willbutler42 commented 1 year ago

If using pseudoyears, the lookup formulas generated in g3s_time can be scrambled. For instance, if the year is 40 and the step is 1, g3s_time_convert will produce 401, g3s_time_multiplier will subsequently produce 1L which removes cur_step from the lookup formula.

The changes make the multiplier (used in g3_time_convert) a function of the number of characters in year and step. This means that for g3s_time, the times argument is replaced with a time_data dataframe. This is because the multiplier is a function of year and step, rather than a lookup function.

willbutler42 commented 1 year ago

I think the answer here is to simplify. This business with mult was mostly to keep me sane whilst reading the raw numbers---202002 is hard work on your eyes. However, nowadays all the axes have proper labels, so you shouldn't be looking at the raw integers that much.

I'd suggest:

* `mult` is always `10`, regardless of the existence of the step column or number of digits in the year.

* Any `step > 99` is an error (should be enough for anyone)

* In `g3s_time_labels`(), don't print a step if `step == 0`.

Hopefully this should fix the problem by mostly throwing code away.

Another option is to investigate the tuples used by g3_param_table. Having a key of (year, step) is the correct way to do this, but there's not a generic framework for using them, unlike the intlookup. #22 is suggesting doing this.

It'd be a lot bigger job than you're necessarily hoping for, and especially in the R case not particularly fast---the closest equivalent to a tuple in R I could find is paste(year, step, collapse = '.'). I think it can wait for another day.

Thanks, @lentinj . I will follow the fixed mult suggestion and test it out. Agree, the tuples can wait for another day.

lentinj commented 1 year ago

Sounds good. And sorry it took so long to get back to you!

willbutler42 commented 1 year ago

This should be fixed now. Following your suggestion, mult is now fixed to 100 and if step is > 99 an error is issued. I removed g3s_time_multiplier though maybe that could be turned into a parameter?

lentinj commented 1 year ago

I removed g3s_time_multiplier though maybe that could be turned into a parameter?

I'm not convinced it's worth the faff. You could have a stock_time_mult <- 100 constant at the top of the file which might kee things a bit more readable, and then it's only one place to update if it really needs changing.

willbutler42 commented 1 year ago

I removed g3s_time_multiplier though maybe that could be turned into a parameter?

I'm not convinced it's worth the faff. You could have a stock_time_mult <- 100 constant at the top of the file which might kee things a bit more readable, and then it's only one place to update if it really needs changing.

Just left it as is.