kevinwolz / hisafer

An R toolbox for the Hi-sAFe biophysical agroforestry model
6 stars 4 forks source link

Add functionality to define_hisafe() to modify exportFrequencies #143

Closed kevinwolz closed 4 years ago

kevinwolz commented 4 years ago

Modifying profileNames and exportFrequencies (both within the .sim file) is currently not supported by define_hisafe(). If the user attempts to include either of these in the call, then the following error message is returned: "-- profileNames and exportFrequencies cannot be defined using define_hisafe(). Use the 'profiles' argument of define_hisafe()."

profileNames is modified based on the user's input to the profiles argument of define_hisafe(), and then exportFrequencies is forced to the frequencies define by the global variable SUPPORTED.PROFILES in utils.R.

Since export profiles and their frequencies should typically be the same across all simulations in an experiment and not be included in any of the factorial defining done by define_hisafe(), it still doesn't make sense to handle profilesNames and exportFrequecies like any other variable.

However, to allow users to modify the exportFrequencies on the fly without having to create their own template simulation, a different method should be employed other than forcing these with the global variable.

kevinwolz commented 4 years ago

The solution should retain the simple option of just passing a character vector of profile names to define_hisafe() so that specification of the export frequencies is not required at every call.

Option 1: The profiles argument can accept two types of inputs: (1) a character vector of profiles names, in which case the default exportFrequencies from SUPPORTED.PROFILES are used, or (2) a list of profile name - export frequency combinations. define_hisafe() can check the class of the input and act accordingly based on the class.

Option 2: A new argument, freqs, would be added to define_hisafe() to allow direct specification of exportFrequencies. The default, NULL, would indicate that the frequencies should be pulled from the defaults in SUPPORTED.PROFILES. Otherwise, a numeric vector the same length as the character vector supplied to profiles must be supplied. To ensure the supplied frequencies are sensical, there would have to be some complex checking, though this would only work for profiles that hisafer is familiar with, not user-created profiles.

In either case, define_hisafe() would now have to save both the names and frequencies to the hip (reading the defaults from the global variable as necessary). build_hisafe() would then build based on this content and not need to look to SUPPORTED.PROFILES itself.

kevinwolz commented 4 years ago

Option 2 seems like a simpler, cleaner solution. Thoughts @mariegosme or @Guillaume-Blanchet?

kevinwolz commented 4 years ago

I've implemented a fix for this using Option 2 above. I like the way this works, but the big question that remains for me is: What are the allowed values of exportFrequencies for each profile type? I have asked this question on the Hi-sAFe Github: https://github.com/hisafe/hisafe/issues/62

Until I know this answer for sure, I am waiting to implement a more robust validation of user-supplied values to the freqs argument. So, @ThomasGendron, you should now be able to use this functionality if you reinstall hisafer from GitHub (update to hisafer v1.4.23), but use it with caution until these internal validations are implemented.

kevinwolz commented 4 years ago

Instead of created robust profile-specific internal checks, I decided just to better document what the allowed and special values are for freqs in define_hisafe().