[Crop and Soil][Manure] Reflecting dailyspread in CS#2623
[Crop and Soil][Manure] Reflecting dailyspread in CS#2623matthew7838 wants to merge 187 commits into
Conversation
|
Current Coverage: 99% Mypy errors on cs-dailyspread branch: 1678 |
|
🚨 Please update the changelog. This PR cannot be merged until |
|
Current Coverage: 99% Mypy errors on cs-dailyspread branch: 1710 |
|
🚨 Please update the changelog. This PR cannot be merged until |
|
Current Coverage: 99% Mypy errors on cs-dailyspread branch: 1710 |
|
🚨 Please update the changelog. This PR cannot be merged until |
|
Current Coverage: 99% Mypy errors on cs-dailyspread branch: 1709 |
|
🚨 Please update the changelog. This PR cannot be merged until |
|
Current Coverage: 99% Mypy errors on cs-dailyspread branch: 1677 |
|
🚨 Please update the changelog. This PR cannot be merged until |
|
Current Coverage: 99% Mypy errors on cs-dailyspread branch: 1684 |
|
🚨 Please update the changelog. This PR cannot be merged until |
|
Current Coverage: % Mypy errors on cs-dailyspread branch: 1685 |
|
🚨 Please update the changelog. This PR cannot be merged until |
# Conflicts: # RUFAS/biophysical/field/field/field.py # RUFAS/biophysical/manure/manure_manager.py # RUFAS/biophysical/manure/storage/daily_spread.py # RUFAS/data_structures/events.py # RUFAS/data_structures/manure_to_crop_soil_connection.py # input/metadata/properties/default.json # tests/test_biophysical/test_manure/test_manure_manager/test_manure_manager.py # tests/test_biophysical/test_manure/test_storage/test_daily_spread.py
|
Current Coverage: 99% Mypy errors on cs-dailyspread branch: 1140 |
|
🚨 Unauthorized changes detected in protected files. Please remove these changes if they are not intended. |
|
Current Coverage: 99% Mypy errors on cs-dailyspread branch: 1140 |
|
🚨 Unauthorized changes detected in protected files. Please remove these changes if they are not intended. |
ew3361zh
left a comment
ew3361zh
left a comment
There was a problem hiding this comment.
Close to approving, just needed to run a few tests to see what happens when you try to run daily spread with no manure module (e.g. field_only simulation type).
Great work and thanks for doing a walk through with us in the dev team wt.
I do think a diagram of the logic would be helpful here in terms of how manure requests are fulfilled based on whether daily spread is happening and also what the nutrient request fulfillment looks like from the manure module when there are BOTH daily spread requests and "normal" manure schedule request events.
| "is_daily_spreading": { | ||
| "type": "bool", | ||
| "description": "Whether this field uses daily spread manure applications.", | ||
| "default": false | ||
| }, | ||
| "spread_all_available_manure": { | ||
| "type": "bool", | ||
| "description": "If true, spread all manure available in DailySpread processors that day, ignoring nutrient targets, caps, manure type, and supplemental manure settings.", | ||
| "default": false | ||
| }, |
There was a problem hiding this comment.
Is the hierarchy that is_daily_spreading being false will override all the other values here and render them irrelevant?
Just want to be sure because it seems like spread_all_available_manure could potentially be of interest to non-daily-spread manure applications.
There was a problem hiding this comment.
I think that spread-all itself is a subtype of daily-spread. Spread all means spread all daily spread processors; therefore, I think the case you mentioned isn't valid.
| if request.use_daily_spread_source: | ||
| manure_source = "daily_spread" | ||
| daily_spread_storages, _ = self._split_storages_by_daily_spread() | ||
| request_result, is_nutrient_request_fulfilled = self._handle_nutrient_request_for_storages( | ||
| request=request, storages=daily_spread_storages | ||
| ) | ||
| if request_result is not None: | ||
| self._remove_nutrients_from_storage( | ||
| request_result, | ||
| request.manure_type, | ||
| include_daily_spread=True, | ||
| update_nutrient_manager_pool=False, | ||
| ) | ||
| else: | ||
| manure_source = "stored_manure" | ||
| request_result, is_nutrient_request_fulfilled = self._manure_nutrient_manager.handle_nutrient_request( | ||
| request | ||
| ) | ||
| if request_result is not None: | ||
| self._remove_nutrients_from_storage( | ||
| request_result, | ||
| request.manure_type, | ||
| include_daily_spread=False, | ||
| ) |
There was a problem hiding this comment.
Slight preference for this to reduce duplication and it's a little clearer to me but not a required change for me.
| if request.use_daily_spread_source: | |
| manure_source = "daily_spread" | |
| daily_spread_storages, _ = self._split_storages_by_daily_spread() | |
| request_result, is_nutrient_request_fulfilled = self._handle_nutrient_request_for_storages( | |
| request=request, storages=daily_spread_storages | |
| ) | |
| if request_result is not None: | |
| self._remove_nutrients_from_storage( | |
| request_result, | |
| request.manure_type, | |
| include_daily_spread=True, | |
| update_nutrient_manager_pool=False, | |
| ) | |
| else: | |
| manure_source = "stored_manure" | |
| request_result, is_nutrient_request_fulfilled = self._manure_nutrient_manager.handle_nutrient_request( | |
| request | |
| ) | |
| if request_result is not None: | |
| self._remove_nutrients_from_storage( | |
| request_result, | |
| request.manure_type, | |
| include_daily_spread=False, | |
| ) | |
| if request.use_daily_spread_source: | |
| manure_source = "daily_spread" | |
| daily_spread_storages, _ = self._split_storages_by_daily_spread() | |
| request_result, is_nutrient_request_fulfilled = self._handle_nutrient_request_for_storages( | |
| request=request, | |
| storages=daily_spread_storages, | |
| ) | |
| include_daily_spread = True | |
| update_nutrient_manager_pool = False | |
| else: | |
| manure_source = "stored_manure" | |
| request_result, is_nutrient_request_fulfilled = self._manure_nutrient_manager.handle_nutrient_request( | |
| request | |
| ) | |
| include_daily_spread = False | |
| update_nutrient_manager_pool = True | |
| if request_result is not None: | |
| self._remove_nutrients_from_storage( | |
| request_result, | |
| request.manure_type, | |
| include_daily_spread=include_daily_spread, | |
| update_nutrient_manager_pool=update_nutrient_manager_pool, | |
| ) |
| @@ -984,17 +1071,114 @@ def _remove_nutrients_from_storage(self, results: NutrientRequestResults, manure | |||
| "bedding_non_degradable_volatile_solids", | |||
| ] | |||
There was a problem hiding this comment.
Looking for ways to reduce the size of this function and I wonder if we could move these to be a class/file-level constant?
| storage_manure_type = STORAGE_CLASS_TO_TYPE.get(type(storage)) | ||
| if storage_manure_type != manure_type: | ||
| continue | ||
| source_stream = ( | ||
| storage.available_for_field_application if isinstance(storage, DailySpread) else storage.stored_manure | ||
| ) |
There was a problem hiding this comment.
Maybe extract to a helper function since it's the same code here and in the other loop?
| non_daily_storages.append(processor) | ||
| return daily_spread_storages, non_daily_storages | ||
|
|
||
| def _handle_nutrient_request_for_storages( |
There was a problem hiding this comment.
If this is just for daily_spread related requests (as it seems like it may be from the one place it's used), should we add that either into the function name or the docstrings for clarity?
| for storage in storages: | ||
| storage_manure_type = STORAGE_CLASS_TO_TYPE.get(type(storage)) | ||
| if storage_manure_type != request.manure_type: | ||
| continue | ||
| source_stream = ( | ||
| storage.available_for_field_application if isinstance(storage, DailySpread) else storage.stored_manure | ||
| ) | ||
| nitrogen += source_stream.nitrogen | ||
| phosphorus += source_stream.phosphorus | ||
| potassium += source_stream.potassium | ||
| total_manure_mass += source_stream.mass | ||
| dry_matter += source_stream.total_solids |
There was a problem hiding this comment.
This same logic is repeated above, I think it could be extracted into a helper function.
ew3361zh
left a comment
ew3361zh
left a comment
There was a problem hiding this comment.
Tested a daily spread manure schedule with a simulation type that had no manure module and it didn't crash which I think is good (field-only). I can't speak to the scientific outcomes for manure splitting and whether it's retrieving manure and dividing it between the existing manure pools as intended so will leave that to the SMEs but code-wise this seems to be working as designed and no further feedback from me.
|
Current Coverage: 99% Mypy errors on cs-dailyspread branch: 1134 |
|
🚨 Unauthorized changes detected in protected files. Please remove these changes if they are not intended. |
|
Current Coverage: 99% Mypy errors on cs-dailyspread branch: 1134 |
|
🚨 Unauthorized changes detected in protected files. Please remove these changes if they are not intended. |
4 participants
Context
Issue(s) closed by this pull request: closes #2494
What
This branch adds a new daily_spread block to a field’s manure schedule and wires it into the simulation as a daily manure request. When the field is created, FieldManager still builds the normal scheduled manure events, but it also pulls out the raw daily_spread settings and stores them on the Field. Then, each day, the field checks whether daily spreading is enabled and, if so, creates a synthetic ManureEvent for that day.
That synthetic event uses the daily_spread nutrient settings to set the request amount. In the current code, only nitrogen_spread_amount and phosphorus_spread_amount are actually used, each capped by max_nitrogen and max_phosphorus. The event is tagged as is_daily_spread, and when it is converted into a NutrientRequest, that tag becomes use_daily_spread_source=True. That flag is what changes the downstream logic.
In ManureManager, requests marked as daily spread do not use the normal shared manure pool first. Instead, the manager splits storages into DailySpread versus regular storages, tries to fulfill the request only from the DailySpread processors, and removes nutrients from their available_for_field_application stream. The DailySpread storage itself was changed so that after it processes manure for the day, it keeps that manure in a same-day available bucket rather than passing it along like normal stored manure.
So the overall logic is: input daily_spread settings on the field, turn them into a daily manure event, convert that into a nutrient request marked as daily-spread-sourced, and fulfill that request from DailySpread storage only. One important gap is that the schema includes potassium_spread_amount and max_potassium, but the current request flow never uses them, because the event and nutrient-request objects only carry nitrogen and phosphorus.
Why
How
Test plan