Updated minimum intake of lactating and dry cows to literature-based bound#2793
Updated minimum intake of lactating and dry cows to literature-based bound#2793
Conversation
Import OutputManager and add lightweight DMI clipping telemetry to ManureExcretionCalculator. Introduces class-level counters and a _track_and_warn_dmi_clip static method that records total/below-min/clipped counts per animal kind (lact/dry), emits a single aggregated warning per kind via OutputManager.add_warning, and raises on unexpected kinds. Functions calculate_lactating_cow_manure and calculate_dry_cow_manure now preserve the original dry_matter_intake in dry_matter_intake_original, apply the existing minimum DMI clipping, and call the tracker with context information. This reduces log spam while providing cumulative diagnostics about DMI clipping behavior.
github-actions
bot
force-pushed
the
refine-minimum-dry-dmi
branch
from
8665d4a to
5594c26
Compare
|
Current Coverage: 99% Mypy errors on refine-minimum-dry-dmi branch: 1684 |
|
🚨 Please update the changelog. This PR cannot be merged until |
Remove the separate n_below_min counter and simplify DMI clipping logic in ManureExcretionCalculator.
Warnings are now emitted only when clipping is applied (dmi < floor), at most once per animal class, and the warning text/label was updated ("minimum floor" → "literature minimum" and warning title changed).
The cumulative message now reports only clipped counts and the _dmi_clip_stats entries drop the n_below_min field.
…armSystems/RuFaS into refine-minimum-dry-dmi
|
Current Coverage: 99% Mypy errors on refine-minimum-dry-dmi branch: 1686 |
|
🚨 Please update the changelog. This PR cannot be merged until |
I updated for dry cows too
…armSystems/RuFaS into refine-minimum-dry-dmi
|
Current Coverage: 99% Mypy errors on refine-minimum-dry-dmi branch: 1686 |
|
🚨 Please update the changelog. This PR cannot be merged until |
|
Current Coverage: 99% Mypy errors on refine-minimum-dry-dmi branch: 1686 |
|
🚨 Please update the changelog. This PR cannot be merged until |
|
To evaluate the PR, I went through 2 series of tests:
Given that the DMI of lactating cows has such as narrow variation, in normal scenarios, it would be unlikely for it to drop below 3.94 kg/d per cow, and similarly for dry cows.
dev + stress (factor=0.2): • LAC manure VS: 30,990 kg/yr PR #2793 + stress (factor=0.2): • LAC manure VS: 31,084 kg/yr These results showed that PR #2793’s higher intake floors (3.94 lact, 7.1 dry) increase the clamped DMI compared to dev, especially for CLOSE_UP/dry cows → higher VS and slightly higher storage CH₄. I also got the warning messages in output/logs as follows: "Predicted DMI for manure excretion is below the literature minimum for dry cows (DMI=3.277 kg/d < 7.100 kg/d). Clipping applied to 7.100 kg/d. Cumulative so far: clipped 1/1 (100.0%)." |
|
Current Coverage: 99% Mypy errors on refine-minimum-dry-dmi branch: 1686 |
github-actions
bot
force-pushed
the
refine-minimum-dry-dmi
branch
from
884543d to
a55e7ba
Compare
|
Current Coverage: 99% Mypy errors on refine-minimum-dry-dmi branch: 1426 |
|
🚨 Flake8 linting errors were found. Please fix the linting issues. |
Wrap and tidy docstrings for DMI-related constants in RUFAS/biophysical/animal/animal_module_constants.py (MINIMUM_DMI_LACT, MINIMUM_DMI_DRY, MINIMUM_DMI_HEIFER, MINIMUM_DMI_CALF, MINIMUM_DMI_LACT_FOR_MANURE_VS, MINIMUM_DMI_DRY_FOR_MANURE_VS). Adjusted line breaks and removed trailing whitespace for consistency; no functional changes.
|
Current Coverage: 99% Mypy errors on refine-minimum-dry-dmi branch: 1426 |
| dry_matter_intake = nutrient_amounts.dry_matter | ||
| dmi_predicted = nutrient_amounts.dry_matter | ||
| dry_matter_intake = dmi_predicted | ||
| dry_matter_intake = max(dry_matter_intake, AnimalModuleConstants.MINIMUM_DMI_LACT) |
There was a problem hiding this comment.
I'm not sure if I misunderstood where we landed in discussing the global vs. literature minimums, but my thought here is if we've set a global minimum for DMI and the predicted DMI is below that, we should not be clipping DMI for manure excretion and instead should be returning an error. The same applies to dry cows below.
There was a problem hiding this comment.
Hi @elle-andreen! Great point here.
After checking with @KFosterReed this morning, we decided to leave the clipping logic inside the manure excretion calculator for now and remove it when we implement the clipping logic where DMI is predicted.
The clipping unlikely changes any downstream results given how low these MINIMUM values are.
That said, I could not recall what was decided for the error messages when predicted DMI is below average literature minimum (and if there will be any error messages). I have recorded my notes from the other day in #434. I agree with you that we could implement an error message if the predicted DMI is below average literature minimum, and I can draft an issue for it but that will be in a separate PR.
There was a problem hiding this comment.
If we want absolutely no risks of changing model behavior, I can set the values of MINIMUM_DMI_LACT and MINIMUM_DMI_DRY as 2 kg/d per cow in this PR and leave animal type-specific minimum DMI values to be updated in issue #2832.
| floor = AnimalModuleConstants.MINIMUM_DMI_LACT_FOR_MANURE_VS | ||
| else: | ||
| floor = AnimalModuleConstants.MINIMUM_DMI_DRY_FOR_MANURE_VS |
There was a problem hiding this comment.
This is very confusing to me. We're providing a warning and stats about clipping DMI, including a floor value, but the floor given here is not actually what we're clipping the DMI value to. We're warning the user that DMI is < 7.1 but not informing the user that we've clipped DMI to 6.06 or 3.6, not 7.1 kg. I also see in #2835 that the documentation was updated to list the lower bound as 7.1 kg. Is this the intended behavior?
There was a problem hiding this comment.
Hello @elle-andreen!
Yes—the intent is to provide a warning when the predicted DMI (post-clipping) falls below the equation-based lower bound, while still allowing the simulation to continue (this was the original request in #434). The log message is meant to inform users that manure VS is being calculated using a % of DMI that is below the equation bounds.
The clipping logic currently remains in the manure excretion calculator temporarily. As mentioned in my previous response, it will need to be removed once the clipping logic is implemented at the DMI prediction step, as documented in issue #2832.
To summarize, there are two types of minimum DMI thresholds:
- Universal minimum DMI – currently implemented through clipping in the manure excretion calculator (temporarily, until clipping is moved to the DMI prediction stage).
- Equation-specific minimum DMI – does not affect calculations; it only triggers a warning message in the log when the predicted DMI (post-clipping) falls below the equation bounds.
I hope this clarifies. Thanks!
There was a problem hiding this comment.
Thanks Haowen. I think it's the order of operations here (i.e. what is happening in this PR vs. what is intended to happen long term) that is making this confusing. I would definitely recommend including components of your comment above in the PR description as appropriate, the description reads as if RuFaS-wide DMI bounds are fully implemented in this PR. The 'why' in the PR description being more specific to what is actually happening here would also be helpful for clarity (i.e., adding temporary minimum DMI bounds for manure excretion to prevent calculation errors, and adding more permanent tracking and logging of instances where RuFaS-predicted DMI (after clipping to the global minimum) falls outside the range of DMI values used to develop the VS excretion equation).
Ignoring the clipping portion that will be changing in the future, this looks good to me otherwise! My only remaining suggestion would be to update the docstring for _track_and_warn_dmi_clip to be a little more detailed. I see Niko left some suggestions, I am happy to test this PR on my end once the changes are finalized.
There was a problem hiding this comment.
Hi Elle — I updated the PR description to clarify that we’re temporarily placing the RuFaS-wide minimum DMI within manure_excretion_calculator.py. I also expanded the helper’s docstring and renamed _track_and_warn_dmi_clip to _track_and_warn_dmi_threshold to avoid implying we modify DMI. Let me know if you have further feedback on this PR. Thanks!
ew3361zh
left a comment
ew3361zh
left a comment
There was a problem hiding this comment.
Nice work here Haowen. I left some initial feedback this first round since it looks like there is some conversation still happening on the scientific side. My general feedback would be to keep things simple and within the context of abstraction - only create helper functions/classes/custom typing for things needed in multiple uses/contexts. If it's a one-time use, it's probably fine to just type it using the existing datatypes. And use the existing nomenclature whenever possible - (e.g. use info_maps instead of context).
RUFAS/biophysical/animal/digestive_system/manure_excretion_calculator.py
Outdated
Show resolved
Hide resolved
| @staticmethod | ||
| def _safe_pct(n: int, d: int) -> float: | ||
| return (100.0 * n / d) if d else 0.0 |
There was a problem hiding this comment.
Probably not necessary to create a function here as it looks like it's only used in a warning message. If it's really necessary to check this, I'd do it within the emit_dmi_below_min_summary() method.
There was a problem hiding this comment.
Good point, Niko — I removed _safe_pct and compute the percentage inline in emit_dmi_below_min_summary() since it’s only used there.
| dmi_predicted : float | ||
| Predicted DMI before clipping (kg/day). |
There was a problem hiding this comment.
If this isn't needed in the function, you can leave it out.
| context : dict[str, Any] | ||
| info_map-like context for OutputManager warnings. |
There was a problem hiding this comment.
Same with this, it's unreferenced in the function so it's likely unnecessary to have it as a function arg.
There was a problem hiding this comment.
Good catch, Niko for the last two comments — I removed dmi_predicted and context since they weren’t used, and updated the call sites and docstring accordingly.
| stats["n_below_min"] += 1 | ||
|
|
||
| @staticmethod | ||
| def emit_dmi_below_min_summary(context: dict[str, Any]) -> None: |
There was a problem hiding this comment.
Is there a reason to not call this arg info_map? It would be more consistent with other use in RUFAS.
There was a problem hiding this comment.
Thanks Niko! I updated to info_map for consistency with other OutputManager calls across RUFAS.
Rename the internal helper _track_and_warn_dmi_clip to _track_and_warn_dmi_threshold and update its docstring to clarify that it records occurrences where effective DMI falls below the literature minimum (tracked per herd type) and does not modify DMI values. Update call sites for lactating and dry DMI clipping to use the new name. No functional behavior changes, just naming and documentation improvements for clearer intent.
Remove the _DMIBelowMinStats TypedDict and the _DMI_KIND Literal type; replace with a plain dict[str, dict[str, int]] for _dmi_below_min_stats and use str for the kind parameter in _track_and_warn_dmi_threshold. Also drop unused typing imports (Final, Literal, TypedDict). No behavioral changes, just simplified type annotations.
Remove the _safe_pct static helper and inline the percentage calculation in _track_and_warn_dmi_threshold. The code now computes pct_below with a divide-by-zero guard and updates the warning message to use this local value. Small refactor to simplify the logic while preserving existing behavior.
Simplify ManureExcretionCalculator._track_and_warn_dmi_threshold by removing unused parameters dmi_predicted and context and updating its docstring. Update call sites in lactating and dry cow manure calculations to only pass kind and dmi_effective. This cleans up the API and removes redundant data passing while preserving the existing DMI threshold tracking logic.
Rename ManureExcretionCalculator.emit_dmi_below_min_summary parameter from `context` to `info_map` and forward `info_map` to OutputManager.add_warning. Update unit tests to match the internal rename from `_track_and_warn_dmi_clip` to `_track_and_warn_dmi_threshold`, adjust the test name, and update calls to the new method signature.
…armSystems/RuFaS into refine-minimum-dry-dmi
|
Hi @elle-andreen @ew3361zh — I believe I’ve addressed your review comments (PR description, naming, typing simplifications, docstring updates, and argument cleanup). The PR is updated and ready for another look. Thanks! |
|
Current Coverage: 99% Mypy errors on refine-minimum-dry-dmi branch: 1214 |
|
🚨 Please update the changelog. This PR cannot be merged until |
|
Current Coverage: 99% Mypy errors on refine-minimum-dry-dmi branch: 1214 |
4 participants
This PR
(1) updates the RuFaS-wide minimum dry matter intake (DMI) bound for lactating and dry cows (clipping mechanism; temporarily in manure_excretion_calculator.py, until clipping is moved to the DMI prediction stage);
(2) adds a manure-excretion warning message in the log when the predicted DMI (post-clipping) falls below the empirical domain of the underlying manure equations to address the request in #434, without affecting manure calculations.
Context
Issue(s) closed by this pull request: closes #434
What
Why
How
Update RuFaS-wide minimum DMI constants for lactating and dry cows
Add a manure-excretion warning
Test plan
Input Changes
• N/A
Output Changes
Filter
{ "multiple": [ { "name": "Total milk (kg; last 365 d)", "filters": ["AnimalModuleReporter.report_herd_statistics_data.daily_milk_production"], "vertical_aggregation": "sum", "slice_start": -365 }, { "name": "Milking cows (avg over last 365 d)", "filters": ["AnimalModuleReporter.report_daily_animal_population.num_lactating_cows"], "vertical_aggregation": "average", "slice_start": -365 }, { "name": "Close-up animals (avg over last 365 d)", "filters": ["AnimalModuleReporter.report_daily_animal_population.num_(heiferIIIs|dry_cows)"], "vertical_aggregation": "average", "horizontal_aggregation": "sum", "slice_start": -365 }, { "name": "LAC DMI total (kg/day) avg over last 365 d", "filters": ["AnimalModuleReporter.report_daily_ration_per_pen.ration_daily_feed_totals_.*_LAC_COW"], "variables": ["dry_matter_intake_total"], "vertical_aggregation": "average", "horizontal_aggregation": "sum", "slice_start": -365 }, { "name": "LAC DMI total (kg/day) SD over last 365 d", "filters": ["AnimalModuleReporter.report_daily_ration_per_pen.ration_daily_feed_totals_.*_LAC_COW"], "variables": ["dry_matter_intake_total"], "vertical_aggregation": "SD", "horizontal_aggregation": "sum", "slice_start": -365 }, { "name": "CLOSE_UP DMI total (kg/day) avg over last 365 d", "filters": ["AnimalModuleReporter.report_daily_ration_per_pen.ration_daily_feed_totals_.*_CLOSE_UP"], "variables": ["dry_matter_intake_total"], "vertical_aggregation": "average", "horizontal_aggregation": "sum", "slice_start": -365 }, { "name": "CLOSE_UP DMI total (kg/day) SD over last 365 d", "filters": ["AnimalModuleReporter.report_daily_ration_per_pen.ration_daily_feed_totals_.*_CLOSE_UP"], "variables": ["dry_matter_intake_total"], "vertical_aggregation": "SD", "horizontal_aggregation": "sum", "slice_start": -365 }, { "name": "LAC DMI per cow (kg/cow/d) avg (approx)", "cross_references": [ "LAC DMI total \\(kg/day\\) avg over last 365 d_ver_hor_agg.*", "Milking cows \\(avg over last 365 d\\)_ver_agg.*" ], "horizontal_aggregation": "division", "horizontal_order": [ "LAC DMI total \\(kg/day\\) avg over last 365 d_ver_hor_agg.*", "Milking cows \\(avg over last 365 d\\)_ver_agg.*" ] }, { "name": "LAC DMI per cow (kg/cow/d) SD (approx)", "cross_references": [ "LAC DMI total \\(kg/day\\) SD over last 365 d_ver_hor_agg.*", "Milking cows \\(avg over last 365 d\\)_ver_agg.*" ], "horizontal_aggregation": "division", "horizontal_order": [ "LAC DMI total \\(kg/day\\) SD over last 365 d_ver_hor_agg.*", "Milking cows \\(avg over last 365 d\\)_ver_agg.*" ] }, { "name": "CLOSE_UP DMI per animal (kg/animal/d) avg (approx)", "cross_references": [ "CLOSE_UP DMI total \\(kg/day\\) avg over last 365 d_ver_hor_agg.*", "Close-up animals \\(avg over last 365 d\\)_ver_hor_agg.*" ], "horizontal_aggregation": "division", "horizontal_order": [ "CLOSE_UP DMI total \\(kg/day\\) avg over last 365 d_ver_hor_agg.*", "Close-up animals \\(avg over last 365 d\\)_ver_hor_agg.*" ] }, { "name": "CLOSE_UP DMI per animal (kg/animal/d) SD (approx)", "cross_references": [ "CLOSE_UP DMI total \\(kg/day\\) SD over last 365 d_ver_hor_agg.*", "Close-up animals \\(avg over last 365 d\\)_ver_hor_agg.*" ], "horizontal_aggregation": "division", "horizontal_order": [ "CLOSE_UP DMI total \\(kg/day\\) SD over last 365 d_ver_hor_agg.*", "Close-up animals \\(avg over last 365 d\\)_ver_hor_agg.*" ] }, { "name": "LAC manure VS (kg/yr; last 365 d)", "filters": ["AnimalModuleReporter.report_manure_excretions.LAC_COW_.*_volatile_solids"], "vertical_aggregation": "sum", "horizontal_aggregation": "sum", "horizontal_first": false, "slice_start": -365 }, { "name": "Close-up (incl dry cows) manure VS (kg/yr; last 365 d)", "filters": ["AnimalModuleReporter.report_manure_excretions.CLOSE_UP_.*_volatile_solids"], "vertical_aggregation": "sum", "horizontal_aggregation": "sum", "horizontal_first": false, "slice_start": -365 }, { "name": "Total housing manure CH4 (kg/yr; last 365 d)", "filters": ["Manure.SingleStreamHandler.*housing_methane_emissions"], "vertical_aggregation": "sum", "horizontal_aggregation": "sum", "horizontal_first": false, "slice_start": -365 }, { "name": "Total storage manure CH4 (kg/yr; last 365 d)", "filters": ["Manure.Storage.*storage_methane$", ".*methane_leakage_mass"], "vertical_aggregation": "sum", "horizontal_aggregation": "sum", "horizontal_first": false, "slice_start": -365 } ] }