757 dont store pointers in the abm

[reneSchm]

Changes and Information

Please briefly list the changes made, additional Information and what the Reviewer should look out for:

• remove pointers from World, Location, Person
• this may change quite a bit more

Merge Request - Guideline Checklist

Please check our git workflow. Use the draft feature if the Pull Request is not yet ready to review.

Checks by code author

• Every addressed issue is linked (use the "Closes #ISSUE" keyword below)
• New code adheres to coding guidelines
• No large data files have been added (files should in sum not exceed 100 KB, avoid PDFs, Word docs, etc.)
• Tests are added for new functionality and a local test run was successful
• Appropriate documentation for new functionality has been added (Doxygen in the code and Markdown files if necessary)
• Proper attention to licenses, especially no new third-party software with conflicting license has been added
• (For ABM development) Checked benchmark results

abm_benchmark/abm_benchmark_50k        4290 ms         4288 ms            1
abm_benchmark/abm_benchmark_100k       9741 ms         9737 ms            1
abm_benchmark/abm_benchmark_200k      20698 ms        20691 ms            1

Checks by code reviewer(s)

• Corresponding issue(s) is/are linked and addressed
• Code is clean of development artifacts (no deactivated or commented code lines, no debugging printouts, etc.)
• Appropriate unit tests have been added, CI passes and code coverage is acceptable (did not decrease)
• No large data files added in the whole history of commits(files should in sum not exceed 100 KB, avoid PDFs, Word docs, etc.)

[DavidKerkmann]

Thanks for the changes! One idea:
We could omit the parameter LocationType in a lot of places where it is only necessary to identify the location, as the Id is unique for a location. Of course, this needs to stay for functions like get/set_assigned_location.

[khoanguyen-dev]

There are some comments that need to be updated. Otherwise, except for David's comments, it all looks good to me.

[reneSchm]

We could omit the parameter LocationType in a lot of places where it is only necessary to identify the location, as the Id is unique for a location.

My IDE usually complaints if there are unused variables, so I don't think that happens too often in our code. But we probably could rework e.g. the testing strategy, so that it stores two vectors, one for location id and one for type. This way we could also remove all the find-ifs and use constant time lookup instead.
But again, I would prefer opening a new issue instead of doing this in here as well.

[xsaschako]

Quick remark:

In the benchmark get_subpopulation_combined is very slow, this maybe needs to be investigated.

[reneSchm]

In the benchmark get_subpopulation_combined is very slow, this maybe needs to be investigated.

This is partially intentional, as get_subpopulation needs to iterate over all persons instead of all persons at the given location, which we currently do not cache. Although get_subpopulation_combined is in fact a lot slower than it has to be right now, instead of calling get_subpopulation it should iterate over all persons exactly once. Something like:

size_t World::get_subpopulation_combined(TimePoint t, InfectionState s) const
{
    return std::count_if(m_persons.begin(), m_persons.end(), [&](const auto& p) {
        return p.get_infection_state(t) == s;
    });
}

[xsaschako]

Thank you! I mean i don't think its high priority, but as it is in the benchmark debuggging, it becomes unusable. Maybe we just think about a better way to debug the benchmark or implement this. but again, very low priority just something i noticed