Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use host pool and action objects to manage data and operations #184

Merged
merged 103 commits into from
Sep 26, 2023

Conversation

wenzeslaus
Copy link
Member

@wenzeslaus wenzeslaus commented May 6, 2022

Old code design:

  • Original design was trying to recreate the R experience where data is accessed directly (like in the original plain R implementation).
  • Each function had only what it needed and was doing everything related to its operation so that all the behavior would be managed in a single space in the source code.
  • All code was written using need-to-know principle, so classes and function would always get just the bare minimum of data.
  • All action related to spread were in one class (Simulation).
  • Direct access to data lead to difficult updates to the model when additional data was used because all functions required review.

New code design:

  • Data is hidden rather than directly exposed to ensure data consistency.
  • Host pool which hides most of the data implements whatever details are needed to ensure consistency.
  • Who need to manipulate with the data, needs to get the host pool object which contains but at the same time hides all the data.
  • Each individual function (feature, process) is now an action.
  • Action classes take configuration in the constructor and all have a function called action which takes a host pool.

Code:

  • The original, large Simulation class is now broken down into multiple small action classes.
  • The SpreadAction class is somewhat close to the very early versions of the Simulation class which was doing only the disperser generation and spread. To allow use from Simulation where these two are separate, SpreadAction uses these two phases in the implementation and exposes the two relevant methods.
  • To avoid circular dependencies, environment and host pool have an interface (class with purely abstract methods) which they are using. (Host pool is using weather from environment for establishment and environment uses host total to compute total populations.)

Compatibility:

  • The Simulation class is now using the actions to do its work, so there are only minimal changes to the Model class.
  • The Model class can eventually use the action classes directly making the Simulation deprecated (but still useful for the existing tests).
  • The treatment classes are now using HostPool to manage the data just like the action classes. The treatment interface is changed slightly because mortality does not have to managed directly from the Model, instead it is resolved inside treatments.

Last phase pieces:

  • Mortality in treatments is still managed by modifying the data directly.
  • Design for tracking outside, generated and established dispersers.
  • See if the Action classes need common base with virtual methods.
  • Decide on random number generators being in function (method) parameters versus passed in the constructor. Host pool methods are used in different contexts, so it seems passing the generator as late as possible is necessary. The actions are what mostly overlaps with features and separate seeds, so they can be have it associated in the constructor. With Separate seeds and random number generators #192, actions can take it anywhere and just decide, but host pool would not know the context, so a specific generator would have to be passed. In any case, the type of the RNG needs to be defined on class level for class templates because virtual methods can't be templates.
  • Merge the completely_remove_* methods into one.
  • Simplify host interface.
  • Remove Hosts pseudo-code.
  • Rename the Pests class to PestPool or something else in singular (leave plurals for lists etc.).
  • Use mortality or new generator in lethal temperature action to remove infected which now newly modifies mortality. Original remove for lethal temperature did not touch mortality. Now it modifies mortality cohorts and host pool is using a random draw to do that. Hence, a generator is needed for mortality or for temperature.
  • Add documentation.

The results from tests for core and for r.pops.spread are identical with the new code.

For now, this includes also the original pseudo code for host object and actions.

Note: The separate seeds and generators PR (#192) required a lot of changes, so the merge was not trivial.

@wenzeslaus
Copy link
Member Author

Current state in code

20220504_165705

Includes an intermediate state of soils addition.

With a Hosts object

20220506_152446

Includes a concept for the soils addition with a more appropriate behavior.

… explicit total host, resistant, and suitable cells in host pool, so suitable cells needs to be non-const now in all functions
… but determines that (or when) it should happen.
… needed, i.e., weather is disabled but the function still requires that to be a parameter (usage of empty env is currently undefined behavior)
…witch to environment so that the caller does not need to care about weather being enabled or if the multiplication is the right thing to do.
Also changes order of exposed and infected in the parameters. Keeps old one parameter version of disperse (as a wrapper) for tests.
…ate parameter to be associated with class, not method), use using for host class template instantiation
…hich is only what environment needs. This greatly simplifies the code and removes the cyclic dependency between env iface and host iface.
@wenzeslaus
Copy link
Member Author

For the record: Alternative idea with mortality in the action

My initial idea was to do mortality in the action to keep the special action to the action object, but I ended up implementing mortality in the host pool object and here, just call the right method because mortality behavior is linked to the host. Eventually, even mortality_rate and mortality_time_lag may end up in the host for multi-host because they are linked to the to the host.

class Mortality
{
    void action(Hosts host)
    {
        for (const auto& [i, j] : hosts.suitable_cells) {
            for (host in hosts) {
                for (item in hosts.mortality_at(i, j)) {
                        if (index == 0)
                            mortality = item;
                        else
                            mortality = mortality_rate * item;
                        list.push_back(mortality);
                    }
                hosts.kill_at(i, j, mortality);
                }
        }
    }
};

@wenzeslaus wenzeslaus marked this pull request as ready for review August 30, 2023 02:12
Copy link
Member

@ChrisJones687 ChrisJones687 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have skimmed over most of the code and read the actions, pest and host pool more closely. It looks good and passes all tests in rpops.

Co-authored-by: Anna Petrasova <kratochanna@gmail.com>
@wenzeslaus wenzeslaus merged commit f5a69e0 into main Sep 26, 2023
9 checks passed
@wenzeslaus wenzeslaus deleted the hosts-pests-actions branch September 26, 2023 18:06
@wenzeslaus wenzeslaus changed the title Use design with host pool object and action objects Use host pool and action objects to manage data and operations Sep 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request refactoring Code needs refactoring
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants