[Output][Naming][Units] Updates to variable names and units#3149
[Output][Naming][Units] Updates to variable names and units#3149ew3361zh wants to merge 19 commits into
Conversation
|
Current Coverage: 99% Mypy errors on output-variable-names branch: 1136 |
|
🚨 Please update the changelog. This PR cannot be merged until |
|
Current Coverage: 99% Mypy errors on output-variable-names branch: 1136 |
|
Current Coverage: 99% Mypy errors on output-variable-names branch: 1136 |
|
Current Coverage: 99% Mypy errors on output-variable-names branch: 1141 |
allisterakun
left a comment
allisterakun
left a comment
There was a problem hiding this comment.
LGTM! Just one observation regarding if it makes sense to sum L/ha together.
| diesel_consumption_tractor_implement_liter_per_ha, | ||
| ) | ||
| total_diesel_consumption_tractor_implement_liter_per_ha = diesel_consumption_tractor_implement_liter_per_ha | ||
| total_diesel_consumption_tractor_implement_liter_per_ha += diesel_consumption_tractor_implement_liter_per_ha |
There was a problem hiding this comment.
Worth some review here about the meaning of this variable vs its unit:
it seems like we're iterating all tractor operations potentially across difference field (hence different field size). It looks like we are just recording it as an output at the moment, but if we multiply this consumption rate in L/ha by the field size, then it introduces a potential error here.
There was a problem hiding this comment.
After discussion with Pooya and Allister, we decided to change this to an aggregated amount of diesel consumed by all implements across all farms instead trying to summarize different rates across different fields and implements. The total diesel consumed seemed like a more meaningful output. The individual diesel consumption rates are still logged to OM as before (and it's now clearer that they are diesel consumption rates).
|
Current Coverage: 99% Mypy errors on output-variable-names branch: 1141 |
| @@ -173,9 +174,9 @@ def estimate_all(simulate_animals: bool, simulate_feed: bool, simulate_fields: b | |||
| harvest_type=harvest_type, | |||
| ) | |||
|
|
|||
| diesel_consumption_tractor_implement_liter_per_ha = estimator.calculate_diesel_consumption( | |||
| diesel_consumption_tractor_implement_liters_per_ha = estimator.calculate_diesel_consumption( | |||
| diesel_consumption_data_item.get("crop_yield", 0), | |||
| diesel_consumption_data_item["field_production_size"], | |||
| field_production_size, | |||
| tractor, | |||
| diesel_consumption_data_item.get("clay_percent", 0), | |||
| diesel_consumption_data_item.get("mass"), | |||
| @@ -185,21 +186,23 @@ def estimate_all(simulate_animals: bool, simulate_feed: bool, simulate_fields: b | |||
| diesel_consumption_data_item, | |||
| herd_size, | |||
| tractor.tractor_size, | |||
| diesel_consumption_tractor_implement_liter_per_ha, | |||
| diesel_consumption_tractor_implement_liters_per_ha, | |||
| ) | |||
| total_diesel_consumption_tractor_implement_liters += ( | |||
| diesel_consumption_tractor_implement_liters_per_ha * field_production_size | |||
| ) | |||
| total_diesel_consumption_tractor_implement_liter_per_ha = diesel_consumption_tractor_implement_liter_per_ha | |||
| om.add_variable( | |||
| "total_diesel_consumption_tractor_implement", | |||
| total_diesel_consumption_tractor_implement_liter_per_ha, | |||
| {**base_info_map, **{"units": MeasurementUnits.LITERS_PER_HA}}, | |||
| "total_tractor_implement_diesel_consumption", | |||
| total_diesel_consumption_tractor_implement_liters, | |||
| {**base_info_map, **{"units": MeasurementUnits.LITERS}}, | |||
| ) | |||
There was a problem hiding this comment.
@KFosterReed wanted to tag you in here for a discussion/question we weren't able to resolve at our dev team work-together yesterday:
this variable being added at the end (now total_tractor_implement_diesel_consumption) is currently on dev as total_diesel_consumption_tractor_implement. It seems from code-context that it was supposed to be aggregating the individual diesel_consumption_tractor_implement_liter_per_ha (calculated a few lines above from calculate_diesel_consumption()) but the way it was set up it was just resetting every loop and logging the individual calculated value.
I initially made a quick change to aggregate these values and report the total summed amount but as @allisterakun caught in his review, these individual calculated diesel_consumption_tractor_implement_liter_per_ha variables are 1. diesel consumption RATES specific to their tractor implement and a field size and 2. already reported individually by report_diesel_consumption() which is called directly above.
In our discussion, we thought the thing that made sense to calculate was the total diesel consumed across all fields by all implements which meant calculating the rate and multiplying it by the field size and then adding that to the aggregating variable (total_tractor_implement_diesel_consumption).
But we couldn't find the documentation to check if this assumption was correct so we're hoping you can weigh in on this. Let me know if you need more context (or less) or if you have any further clarifications.
3 participants
Addresses a handful of units and variable naming issues raised.
Context
Issue(s) closed by this pull request: closes #3057
What
Weather.mean_annual_temperature->Weather.long_term_average_annual_temperatureto better represent the calculation made to determine it.EnergyEstimator.estimate_all.application_depth_for_(Fertilizer/Manure)_Applicationfromcmtomm.EnergyEstimator.estimate_all.diesel_consumption_for_(Fertilizer_Application/Manure_Application/Harvest/PlantingfromLITERS_PER_TONtoLITERS_PER_HA.EnergyEstimator.estimate_allto properly aggregatetotal_diesel_consumption_tractor_implementas a total amount of diesel consumed across fields and implements and updated the unit toLITERS.AnimalModuleReporter.report_manure_excretions.[ANIMAL_COMBINATION][PEN#]_inorganic_phosphorus_fractionAnimalModuleReporter.report_manure_excretions.[ANIMAL_COMBINATION][PEN#]_organic_phosphorus_fractionAnimalModuleReporter.report_manure_excretions.[ANIMAL_COMBINATION][PEN#]_non_water_inorganic_phosphorus_fractionAnimalModuleReporter.report_manure_excretions.[ANIMAL_COMBINATION][PEN#]_non_water_organic_phosphorus_fractionWhy
The updated units and names were either confusing or outright incorrect.
The removed outputs were not passed to the manure module and the SME team opted to remove them for the time being.
How
Updated specific references as outlined in the
Whatsection. Additionally updated all related unit testing.Test plan
Input Changes
Output Changes
Filter