Conversation
|
Current Coverage: % Mypy errors on fix-mcf-transcription-error branch: 1164 |
|
🚨 Some tests have failed. |
tomhuhh
left a comment
tomhuhh
left a comment
There was a problem hiding this comment.
Thanks for making this update. I had two clarification points that may need SME (@elle-andreen, @KFosterReed) input:
-
For unmixed bedded pack, how should we handle exactly storage_time_period == 30 days / 1 month? My reading is that both IPCC 2019 and the 2024 USDA Bluebook specify factors for < 1 month and > 1 month, but do not explicitly define the exactly-1-month case. The current implementation treats 30 days as the long-duration case; I think we should agree on/document that choice.
-
Which temperature should be passed into the MCF lookup: indoor barn-adjusted temperature or ambient annual temperature? Since bedded pack is an indoor housing system, using _determine_barn_temperature(...) may be consistent with other RuFaS housing systems. But IPCC climate-zone classification appears to be based on ambient climate variables, so I think this needs SME confirmation for traceability.
| (18, math.inf): 1.5, | ||
| } | ||
|
|
||
| BEDDED_PACK_MCF_UNMIXED_SHORT: dict[tuple[float, float], float] = { |
There was a problem hiding this comment.
I would be explicit about the variable name - BEDDED_PACK_MCF_UNMIXED_LESS_THAN_A_MONTH.
There was a problem hiding this comment.
Since a month is not a concrete number of days, would you be okay with BEDDED_PACK_MCF_UNMIXED_UNDER_30_DAYS?
| (18, math.inf): 18.0, | ||
| } | ||
|
|
||
| BEDDED_PACK_MCF_UNMIXED_LONG: dict[tuple[float, float], float] = { |
There was a problem hiding this comment.
Similar here - BEDDED_PACK_MCF_UNMIXED_MORE_THAN_A_MONTH
There was a problem hiding this comment.
And here it would be something like BEDDED_PACK_MCF_UNMIXED_30_DAYS_OR_MORE (or worded however we decide the exact breakdown for where 30 days storage time falls)?
There was a problem hiding this comment.
Sounds good to me. Thanks Niko!
|
Current Coverage: 99% Mypy errors on fix-mcf-transcription-error branch: 1164 |
2 participants
Adds option for
BeddedPackMCF lookup table to account for shorter-term storage duration.Context
Issue(s) closed by this pull request: closes #2959
What
Expands lookup table for
BeddedPackMCF to 3 options:BEDDED_PACK_MCF_MIXEDBEDDED_PACK_MCF_UNMIXED_SHORTBEDDED_PACK_MCF_UNMIXED_LONGIf the
BeddedPack.storage_time_periodis > 30 days orNone, it is consideredLONGand if it's < 30 days it is consideredSHORT.Why
Fixes a possible transcription error for
BeddedPackMCF temperature bins as described in the issue.How
Changed the lookup table format to better account for multiple
UNMIXEDoptions based onstorage_time_period.Adjusted the function that determines the factor to take into account
storage_time_periodand passes that attribute value around to allow for its use in the table lookup.Test plan
I ran the
available_simulation_taskstask to ensure all current example simulations are running as expected. I think some SME testing will be helpful to determine the scientific correctness of the fix.Input Changes
Output Changes
Filter