1213 PAIS for ABM - #1587
Conversation
xsaschako
left a comment
xsaschako
left a comment
There was a problem hiding this comment.
Looks good!
Obv no tests yet, but looks fine otherwise. see remarks in comments
| * @brief #Vaccination classes | ||
| * can be used as 0-based index | ||
| */ | ||
| enum class VaccinationClass : std::uint32_t |
There was a problem hiding this comment.
Shouldnt this be VaccinationCount if its Zero,OneorTwo, ThreeOrMore?
Also is it sensible to do OneOrTwo if it's only used for PAIS maybe use a different word for it,e.g. PAISVaccClass?
| Eigen::VectorX<ScalarType> sum = | ||
| Eigen::VectorX<ScalarType>::Zero(Eigen::Index(mio::abm::InfectionState::Count)); | ||
| auto curr_time = sim.get_time(); | ||
| PRAGMA_OMP(for) |
There was a problem hiding this comment.
just a note also to myself but I think we should leave OMP out for loggers atm (as you did), I've only felt they produce more headache than an performance upgrade...
ill take a closer look at them if I do the performance analysis with parallelization
| /** | ||
| * @brief Looger to log the TimeSeries of the number of Person%s that have an active PAIS. | ||
| */ | ||
| struct LogPAIS : mio::LogAlways { |
There was a problem hiding this comment.
LogCurrentlyActiveAmountOfPais?
maybe too longe but I thought about a full amount of which severity etc.
| return m_infection_course.back(); | ||
| } | ||
| else { | ||
| return m_infection_course[m_infection_course.size() - 2]; |
There was a problem hiding this comment.
short comment why this leads to the desired case and why this is true
also: name sounds like over all infections but its about the last infection course no?
Maybe a short assert so in the future, if anything changes we have no access to -1 index.
| * @return Id of the newly created Person. | ||
| */ | ||
| PersonId add_person(const LocationId id, AgeGroup age); | ||
| PersonId add_person(const LocationId id, AgeGroup age, Sex sex = Sex::Male); |
There was a problem hiding this comment.
Maybe we want unisex as I think we model with that 95% of the time. (also this could be political, think "migration")
|
|
||
| void PAIS::update_severity(const Parameters& params, PersonalRandomNumberGenerator& rng, TimePoint t, TimeSpan dt) | ||
| { | ||
| if (severity.empty() || t > severity.back().first) { |
There was a problem hiding this comment.
it should be t >... or? We only proceed when the last update was before t and this is describing this and we return then ( or am I stupid?)
| * @param[in] t TimePoint of check. | ||
| * @returns Antibody level of the Person at the given TimePoint. | ||
| */ | ||
| ScalarType get_antibody_level(TimePoint t) const; |
| * Each value give the probability that a Person with a certain PAISState transitions to another PAISState within a day. | ||
| * The first index is the from state and the second index is the to state. | ||
| */ | ||
| struct PAISTransitionMatrix { |
| } | ||
|
|
||
| void Person::add_new_infection(Infection&& inf) | ||
| void Person::add_new_infection(Infection&& inf, PersonalRandomNumberGenerator& rng, TimePoint t, |
There was a problem hiding this comment.
this is arguably super often called function, maybe overload this for PAIS?
| */ | ||
| mio::abm::PersonId add_test_person(mio::abm::Model& model, mio::abm::LocationId loc_id, | ||
| mio::AgeGroup age = age_group_15_to_34, | ||
| mio::AgeGroup age = age_group_15_to_34, mio::abm::Sex sex = mio::abm::Sex::Male, |
There was a problem hiding this comment.
probably better to insert as last, could be safer, since the enums are after each other?
2 participants
Changes and Information
Please briefly list the changes (main added features, changed items, or corrected bugs) made:
If need be, add additional information and what the reviewer should look out for in particular:
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
Checks by code reviewer(s)