Fix issue 22 by defining new VariableKiosk

[SCiarella]

Closes #22

Previously, running an individual sub-module like DVS_Partitioning in isolation was only possible through VariableKioskTestHelper, a test-only subclass living in utils.py. This PR promotes that capability into a proper VariableKiosk class for diffWOFOST and removes the helper entirely.

The new class accepts an optional external_state_list a list of per-day dicts containing values for variables that would normally come from other sub-modules. Calling kiosk(day) advances to that day's entry and makes the values transparently available through the normal kiosk interface (kiosk["VAR"], kiosk.VAR, "VAR" in kiosk).

Main changes:

• __call__ always returns False so the kiosk has no opinion on whether the simulation should stop. Callers that need to detect exhaustion of external states (like the TestEngine) use the external_states_exhausted property instead. This makes the class usable outside of test contexts too.
• Iteration is index-based, so the original list is never mutated and can be reused: a separate internal dict is built at init time, so the same external_state_list can safely be reused across multiple engine runs (e.g. in a training loop).
• Lookup is dict-based (keyed by date) rather than index-based, so the external_state_list can be sparse: days with entries inject their values, days without entries simply clear current_externals and fall back to whatever is registered in the kiosk by other modules. This makes the class usable in partial-override scenarios where most variables come from the running model, but a few are forced externally.
• EngineTestHelper now checks kiosk.external_states_exhausted explicitly to decide when to stop the test run, keeping that responsibility in the engine rather than the kiosk.
• VariableKiosk subclasses pcse.VariableKiosk
• VariableKioskTestHelper has been removed and EngineTestHelper uses VariableKiosk directly
• Direct tests for VariableKiosk have been added.

[fnattino]

Hi @SCiarella, very nice, I like how this would drop the VariableKioskTestHelper and how external states would be added to the kiosk!

Looking at this, I have been thinking about the following: what if we would move the functionality to update the external states in the kiosk (essentially, what is currently in the VariableKiosk.__call__ method) outside of the kiosk, i.e. at the level of the engine? What I mean is that we could modify the Engine to update the kiosk at each model iteration, as part of the _run method. Something like the following:

class Engine(...):
    def __init__(self, ..., update_external_states):
        ...
        self.kiosk = VariableKiosk()
        self.update_external_states = update_external_states
    ...
    def _run(self):
        """Make one time step of the simulation."""

        # Update timer
        self.day, delt = self.timer()

        # The following updates the external states in the kiosk for each day, 
        # - everything afterwards follow as it is (integration, agromanagement, rate calculation, ...) 
        self.update_external_states(day, self.kiosk)
        ...

An advantage of this approach would be that one can more easily update the state/rate variables, using a rather generic procedure. Until now, one expects the external states for all dates to be known at the beginning of the simulation, but with the approach described above one could use update_external_states in other ways as well ( I am thinking to the discussion on how to replace model components with ML-based emulator)?

What do you think? Of course what I am describing above could be added as part of a new PR!

[SCiarella]

👋 @fnattino

Your suggestion looks clean! I agree with you that it makes more sense for the Engine to update things, while the kiosk should be more of a container and not do too much stuff. And I agree that it is easier for a ML model eventually to just manipulate through the Engine rather than updating all the parameter classes separately.

Yes, let's open a new issue for this suggestion and include it in the development of the new Engine 🚀

[SarahAlidoost]

Hi @SCiarella, very nice, I like how this would drop the VariableKioskTestHelper and how external states would be added to the kiosk!

Looking at this, I have been thinking about the following: what if we would move the functionality to update the external states in the kiosk (essentially, what is currently in the VariableKiosk.__call__ method) outside of the kiosk, i.e. at the level of the engine? What I mean is that we could modify the Engine to update the kiosk at each model iteration, as part of the _run method. Something like the following:
...
An advantage of this approach would be that one can more easily update the state/rate variables, using a rather generic procedure.

Thanks for the suggestion. It sounds like a good idea to move the functionality of updating the kiosk with external states outside of the kiosk. In the current form of Engine, the update function i.e. self.update_external_states(day, self.kiosk) would be called at the initialization time i.e. __init__ of Engine (because this the place where a crop module get initialized and some modules needs external states at the time of the initialization) and later at each simulation time step i.e. _run of Engine.

Until now, one expects the external states for all dates to be known at the beginning of the simulation,

I'm not sure if this is the case because at the beginning of the simulation (initialization) the kiosk selects only the external states of the "first day", see here. And later in _run, it selects the external states based on "day". That said, I agree that in the current Engine, the external states of all time steps are loaded into memory as a dictionary.

but with the approach described above one could use update_external_states in other ways as well ( I am thinking to the discussion on how to replace model components with ML-based emulator)?

The function update_external_states is meant to provide the state variables for running an individual module. If users run the whole wofost model (either all crop modules or combination of crop and ml modules), in principle, the function update_external_states is not needed. The kiosk should take care of the states.

There are two scenarios:

• as an example, users can write a wofost72.py script where "partitioning" is based on a pre-trained ml model and returns the states/rate at each time steps, similar to other crop modules. The function update_external_states is not needed in this case.
• users already run ml_based "partitioning" model outside of the diffwofost environment and store the related states. Then runs the individual "assimilation" model (as an example) which needs those external variables. The external states should be passed to Engine and the function update_external_states update kiosk at each time step. Again, the external states are loaded to memory.

If something not clear, please let me know.

What do you think? Of course what I am describing above could be added as part of a new PR!

[fnattino]

Thanks for the feedback @SCiarella and @SarahAlidoost!

That said, I agree that in the current Engine, the external states of all time steps are loaded into memory as a dictionary.

Right, what I meant is that the current implementation requires all time steps to be provided at the beginning of the simulation, but this is actually not necessary, as only one time step at the time is loaded in the kiosk.

There are two scenarios:

* as an example, users can write a `wofost72.py` script where "partitioning" is based on a pre-trained ml model and returns the states/rate at each time steps, similar to other crop modules. The function `update_external_states` is not needed in this case.

But how would the pre-trained ML model be able to pass the states/rates to the other physical crop model components? I am thinking that we could make it possible via this update_external_states function, letting users be able to hook external models into diffwofost. The function would have access to the kiosk, which contains all shared states/rates, so the ML model could read required inputs from there, run inference, and update the kiosk accordingly at every time step.

* users already run ml_based "partitioning" model outside of the diffwofost environment and store the related states. Then runs the individual "assimilation" model (as an example) which needs those external variables. The external states should be passed to Engine and the function `update_external_states` update kiosk at each time step. Again, the external states are loaded to memory.

Right, you could also use update_external_states to pass "pre-computed" states and rates to the model - but are there use cases where you can pre-calculate states/rates in advance (i.e. knowing in advance how certain model components will behave in time) other than for test purposes?

But I might be overlooking something, good to discuss this in person!

[SarahAlidoost]

@SCiarella Thanks for implementing this. Please see my suggestions, mainly refactoring. Also, can you please make an issue for adding update_external_states to Engine as mentioned here.

[SarahAlidoost]

@SCiarella thanks! 👍

[sonarqubecloud]

Quality Gate failed

Failed conditions
40.6% Coverage on New Code (required ≥ 80%)

See analysis details on SonarQube Cloud