Conversation
|
Current Coverage: 99% Mypy errors on refactor_pen_get_manure_stream branch: 1255 |
|
🚨 Please update the changelog. This PR cannot be merged until |
github-actions
bot
force-pushed
the
refactor_pen_get_manure_stream
branch
from
d75be45 to
223fee2
Compare
github-actions
bot
force-pushed
the
refactor_pen_get_manure_stream
branch
from
223fee2 to
ff05e30
Compare
|
Current Coverage: 99% Mypy errors on refactor_pen_get_manure_stream branch: 1249 |
|
🚨 Please update the changelog. This PR cannot be merged until |
|
Current Coverage: 99% Mypy errors on refactor_pen_get_manure_stream branch: 1249 |
|
🚨 Please update the changelog. This PR cannot be merged until |
…uminantFarmSystems/RuFaS into refactor_pen_get_manure_stream
allisterakun
requested review from
ew3361zh and
matthew7838
and removed request for
ew3361zh
|
Current Coverage: 99% Mypy errors on refactor_pen_get_manure_stream branch: 1249 |
|
Current Coverage: 99% Mypy errors on refactor_pen_get_manure_stream branch: 1249 |
matthew7838
left a comment
matthew7838
left a comment
There was a problem hiding this comment.
LGTM, nice and clean.
| for stream in self.manure_streams: | ||
| general_substream_proportion = float(stream.get("stream_proportion", 1.0)) | ||
| split_ratio = general_substream_proportion * general_stream_proportion | ||
| manure_stream_deposit_split = ( | ||
| general_substream_proportion if parlor_stream_proportion is not None else split_ratio | ||
| ) | ||
| else: | ||
| methane_production_potential = ( | ||
| 0.17 if self.animal_combination in [AnimalCombination.CALF, AnimalCombination.GROWING] else 0.24 | ||
| manure_stream = total_stream.split_stream( | ||
| split_ratio=split_ratio, | ||
| stream_type=StreamType.GENERAL, | ||
| manure_stream_deposition_split=manure_stream_deposit_split, | ||
| ) | ||
| if manure_stream.pen_manure_data is not None: | ||
| manure_stream.pen_manure_data.set_first_processor(str(stream.get("first_processor"))) |
There was a problem hiding this comment.
Just a small thing.Does it make sense to condense this to another method like split_handling
There was a problem hiding this comment.
Yeah I think I agree with Matthew here, I'm leaning towards making this function as high-level/managerial as possible. I think it would help with clarity.
| - [2843](https://github.com/RuminantFarmSystems/MASM/pull/2843) - [minor change] [NoInputChange] [NoOutputChange] Fix Simple `#noqa`s in codebase. | ||
| - [2852](https://github.com/RuminantFarmSystems/MASM/pull/2852) - [minor change] [NoInputChange] [NoOutputChange] Fix AssertionError on `dev`. | ||
| - [2850](https://github.com/RuminantFarmSystems/MASM/pull/2850) - [minor change] [NoInputChange] [NoOutputChange] Refactor `Pen.get_manure_stream()`. | ||
|
|
ew3361zh
left a comment
ew3361zh
left a comment
There was a problem hiding this comment.
Nice work, Allister! Just a couple comments about extracting more things out of that main get_manure_streams() method but otherwise looks good to go.
One suggestion would be to either get a SME review or get the E2E expected results updated on dev and then run E2E testing with this branch to make sure manure is still processing as expected.
| if parlor_stream_proportion is not None or general_stream_proportion < 1.0 or parlor_stream is not None: | ||
| assert parlor_stream is not None, "Parlor stream should not be None if parlor stream proportion is not None" | ||
| assert ( | ||
| parlor_stream_proportion is not None | ||
| ), "Parlor stream proportion should not be None if parlor stream is not None" | ||
| base_parlor_stream_name = f"{self.parlor_stream_name}" if self.parlor_stream_name else "parlor_stream" | ||
| animal_manure_streams[f"{base_parlor_stream_name}_PEN_{self.id}"] = parlor_stream |
There was a problem hiding this comment.
Is this mypy related or could this be done in either _handle_parlor_stream() or _validate_general_manure_stream_proportions() or extracted into a validate_parlor_stream_proportions() method?
| for stream in self.manure_streams: | ||
| general_substream_proportion = float(stream.get("stream_proportion", 1.0)) | ||
| split_ratio = general_substream_proportion * general_stream_proportion | ||
| manure_stream_deposit_split = ( | ||
| general_substream_proportion if parlor_stream_proportion is not None else split_ratio | ||
| ) | ||
| else: | ||
| methane_production_potential = ( | ||
| 0.17 if self.animal_combination in [AnimalCombination.CALF, AnimalCombination.GROWING] else 0.24 | ||
| manure_stream = total_stream.split_stream( | ||
| split_ratio=split_ratio, | ||
| stream_type=StreamType.GENERAL, | ||
| manure_stream_deposition_split=manure_stream_deposit_split, | ||
| ) | ||
| if manure_stream.pen_manure_data is not None: | ||
| manure_stream.pen_manure_data.set_first_processor(str(stream.get("first_processor"))) |
There was a problem hiding this comment.
Yeah I think I agree with Matthew here, I'm leaning towards making this function as high-level/managerial as possible. I think it would help with clarity.
3 participants
Context
Issue(s) closed by this pull request: closes #2595.
This PR breaks down the
Pen.get_manure_streams()method into smaller functions.Input Changes
Output Changes
Filter