[SimulationEngine] Refactor main daily simulation method#2772
[SimulationEngine] Refactor main daily simulation method#2772
Conversation
|
Current Coverage: % Mypy errors on refactor-daily-sim-2 branch: 1690 |
|
🚨 Please update the changelog. This PR cannot be merged until |
There was a problem hiding this comment.
This is a solid refactor with good docstrings added. As you mention in your Notes for Discussion, I wonder how skipping certain methods should work when only some of the modules are being simulated. If it were me, I would add flags for each module to the main simulation method - as I describe in a separate comment - but maybe that is not ideal, as you indicate.
|
Current Coverage: % Mypy errors on refactor-daily-sim-2 branch: 1690 |
|
🚨 Please update the changelog. This PR cannot be merged until |
|
Thanks for getting this started on what looks (with a preliminary glance) like a great improvement! In reading through the current
Perhaps this is still too repetetive and it just adds another giant function? Something to add to the discussion at least :) |
There was a problem hiding this comment.
Looks a lot cleaner, thanks, Niko. A few suggestions and ideas to discuss.
First, it would be easier to track by having the helper functions relocated to the order they're being called.
Second, I have an idea that can be discussed, which is a variant of what Kristan has. Instead of adding parallel/duplicate execution methods per module combination, we can define a single ordered function pipeline and build it once at startup based on enabled module flags. Since this PR already modularized module calls, each module step can be represented as a callable in that sequence. The daily simulation then just executes the precomputed list in order, which avoids per-day flag checks and keeps one canonical execution path.
|
Current Coverage: % Mypy errors on refactor-daily-sim-2 branch: 1690 |
|
🚨 Please update the changelog. This PR cannot be merged until |
|
Current Coverage: % Mypy errors on refactor-daily-sim-2 branch: 1691 |
|
Current Coverage: % Mypy errors on refactor-daily-sim-2 branch: 1251 |
|
🚨 Unauthorized changes detected in protected files. Please remove these changes if they are not intended. |
|
Current Coverage: 99% Mypy errors on refactor-daily-sim-2 branch: 1251 |
|
🚨 Unauthorized changes detected in protected files. Please remove these changes if they are not intended. |
|
Current Coverage: % Mypy errors on refactor-daily-sim-2 branch: 1249 |
|
🚨 Unauthorized changes detected in protected files. Please remove these changes if they are not intended. |
|
Current Coverage: 99% Mypy errors on refactor-daily-sim-2 branch: 1249 |
|
🚨 Unauthorized changes detected in protected files. Please remove these changes if they are not intended. |
|
Current Coverage: 99% Mypy errors on refactor-daily-sim-2 branch: 1213 |
|
🚨 Unauthorized changes detected in protected files. Please remove these changes if they are not intended. |
Refactors
SimulationEngine._daily_simulation()to be a higher-level managerial function for a day in the life on a RuFaS farm. Also starts the process of more clearly accommodating multiple simulation types (i.e. isolating modules to run on their own or in smaller groups than as a whole farm) some of which are discussed in #2671.Context
Issue(s) closed by this pull request: closes #2735
What
_daily_simulation()one of which executes running a full farm (_execute_full_farm_daily_simulation()) and the other of which executes a farm with no animals simulated (_execute_no_animals_daily_simulation()).configinput calledsimualtion_typewhich would determine which of these daily simulation functions would be used. Note, this is meant to overtake thesimulate_animalsconfiginput so that we don't get conflicting inputs.Why
The daily simulation function is overly complicated and busy. To try and figure out the order of operations and data flowing on a farm, it takes way too much effort. This simplifies that top level daily simulation function for clarity and readability. It also will making testing (which I didn't do yet for reasons to be discussed below) much simpler.
Additionally, we want to maintain this clarity of high level farm operations and exchange of data between them while allowing for multiple types of simulations where different modules or groups of modules are isolated. It also allows the model to have its daily operations set at startup and avoids checking boolean flags each simulation day to know whether or not to run a particular module.
How
_daily_simulation()to now have 2 main options:_execute_full_farm_daily_simulation()which runs a full farm simulation with all modules and_execute_no_animals_daily_simulation()where it runs everything from a full farm simulation except for animal operations and manure operations.Notes For Discussion
_execute_daily_simulation()(pka_daily_simulation()) function inSimulationEngine.Test plan
Try running a freestall simulation and a no-animals simulation. Both should run as before.
Unit testing will be expanded to cover all the new functions in SimulationEngine once this refactor approach is approved.
Input Changes
Output Changes
Filter