CNM-University-of-Guelph / NASEM-Model-Python

The NASEM 8 model coded in python
Other
0 stars 0 forks source link

check_coeffs_in_coeff_dict redundant #103

Closed BFieguth closed 2 days ago

BFieguth commented 4 weeks ago

Implementing a validate_coeff_dict function at the start of execute_model makes running check_coeffs_in_coeff_dict redundant as we have already verified that coeff_dict contains all the required keys. Should check_coeffs_in_coeff_dict be deprecated?

It would still be used if a user ran functions from the NASEM_equations directory individually. However, if a key is missing a KeyError should be raised by Python. Would this not be sufficient for the user to debug?

@davos-i what do you think?

davos-i commented 4 weeks ago

I did a quick comparison with a random function: With the check_coeffs_in_coeff_dict:

AssertionError                            Traceback (most recent call last)
Cell In[15], line 1
----> 1 dev_calculate_Dt_DMIn_BW_LateGest_i(-3, -0.27, {})

Cell In[2], line 6, in dev_calculate_Dt_DMIn_BW_LateGest_i(An_PrePartWklim, Kb_LateGest_DMIn, coeff_dict)
      1 def dev_calculate_Dt_DMIn_BW_LateGest_i(An_PrePartWklim, 
      2                                     Kb_LateGest_DMIn,
      3                                     coeff_dict
      4 ) -> float:
      5     req_coeffs = ['Ka_LateGest_DMIn', 'Kc_LateGest_DMIn']
----> 6     nd.check_coeffs_in_coeff_dict(coeff_dict, req_coeffs)
      7     # Late gestation individual animal prediction, % of BW.  Use to assess for a
      8     # specific day for a given animal
      9     Dt_DMIn_BW_LateGest_i = (coeff_dict['Ka_LateGest_DMIn'] + 
     10                              Kb_LateGest_DMIn * An_PrePartWklim + 
     11                              coeff_dict['Kc_LateGest_DMIn'] * An_PrePartWklim**2)

File /NASEM-Model-Python/src/nasem_dairy/ration_balancer/ration_balancer_functions.py:53, in check_coeffs_in_coeff_dict(input_coeff_dict, required_coeffs)
     49 result = not bool(missing_coeffs)
     51 # Raise an AssertionError with a custom message containing missing values 
     52 # if the condition is False
---> 53 assert result, f\"Missing values in coeff_dict: {missing_coeffs}\"
     55 return

AssertionError: Missing values in coeff_dict: ['Kc_LateGest_DMIn', 'Ka_LateGest_DMIn']"

Without the custom checker it shows a key error for only the first missing value:

KeyError                                  Traceback (most recent call last)
Cell In[17], line 1
----> 1 dev_calculate_Dt_DMIn_BW_LateGest_i_2(-3, -0.27, {})

Cell In[16], line 22, in dev_calculate_Dt_DMIn_BW_LateGest_i_2(An_PrePartWklim, Kb_LateGest_DMIn, coeff_dict)
     14 def dev_calculate_Dt_DMIn_BW_LateGest_i_2(An_PrePartWklim, 
     15                                     Kb_LateGest_DMIn,
     16                                     coeff_dict
   (...)
     20     # Late gestation individual animal prediction, % of BW.  Use to assess for a
     21     # specific day for a given animal
---> 22     Dt_DMIn_BW_LateGest_i = (coeff_dict['Ka_LateGest_DMIn'] + 
     23                              Kb_LateGest_DMIn * An_PrePartWklim + 
     24                              coeff_dict['Kc_LateGest_DMIn'] * An_PrePartWklim**2)
     25     return Dt_DMIn_BW_LateGest_i

KeyError: 'Ka_LateGest_DMIn'"

So - I think we could deprecate it but it should be replaced with docstring that has a minimum reproducible example. E.g. for this function it would have:

  """

   Parameters
    ----------

    Returns
    -------

    Examples
    --------
    Calculate the dry matter intake for an individual animal during late gestation:
coeff_dict = {
    'Ka_LateGest_DMIn': 1.47,
    'Kc_LateGest_DMIn': -0.035
}

dev_calculate_Dt_DMIn_BW_LateGest_i_2(
    An_PrePartWklim = -3,
    Kb_LateGest_DMIn = -0.272489,
    coeff_dict = coeff_dict
)
 """

This will be especially important for the functions that take multiple coeff dicts. This whole thing should probably be done alongside decision of how to structure all coeffs (#102)