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

Merge Static and Dynamic risk factors #194

Closed
jamesturner246 opened this issue Aug 2, 2023 · 3 comments
Closed

Merge Static and Dynamic risk factors #194

jamesturner246 opened this issue Aug 2, 2023 · 3 comments

Comments

@jamesturner246
Copy link
Contributor

I've put some thought into this, and it has become clear that the main thing hindering further refactoring -- thus simplifying the process of adding new RF models -- is the separation of Static and Dynamic risk factor models. Some reasons:

  • Both of the existing older RF model S/D pair use common functionality -- e.g. the HierarchicalMapping mechanism that's passed around everywhere, that is not necessarily useful for other RF implementations. This can instead be contained in the new merged RF model class.
  • It is preventing separation of JSON risk factor config from the main config file, since, for example, both static and dynamic models rely on the same list of risk factors in the main config, and moving it from the main config would require duplicating them in the static/dynamic RF configs.
  • Cleaner and more encapsulated code: a *lot of code for handling the separate S and D risk models can be removed, and instead placed inside the class which requires it.

I'll add to this list as I think of more, but it's already starting to become an attractive and clean solution in my head.

@jamesturner246 jamesturner246 converted this from a draft issue Aug 2, 2023
@israel-vieira
Copy link
Contributor

Do you have a new, non-regression based static model for initialization? Perhaps you could use a base class to minimize the duplication, but from memory, the existing static and dynamic have very different sampling methods.

@jamesturner246
Copy link
Contributor Author

jamesturner246 commented Aug 2, 2023

From what I interpret from what Ali has said, there will be a new initialisation method, but you have a point that it's worth considering if this new, as yet unclear, method can be fit into the existing static HLM framework. I will try to get an answer to this in tomorrow's meeting. In that case, I'm imagining a HLM object inside of the merged RF model, which contains the mappings, etcetera, rather than passing them around everywhere via context, which can be used by any RF model, new or old.

Otherwise, despite sampling methods, from what, I can see, the core functionality is still separated into the generate_risk_factors and update_risk_factors methods. In the static model, update_risk_factors is implemented such that it only fires for Persons of age zero, and in that case it behaves identically to generate_risk_factors. In the dynamic model, only update_risk_factors is implemented, with generate_risk_factors left undefined.

@israel-vieira
Copy link
Contributor

In that case, I would wait until the new model structure is defined, try to understand the new model before doing any major consolidation work, that might not even be relevant to the new models.

The current two models MUST share the same risk factors because the first one, S, initialize the virtual population risk factors absolute values and the second, D, updates the population over time by applying deltas to the same risk factors.

The population size is dynamic, new born babies must be initialized by the S model at birth at any point in time during the simulation. The two models looks similar because of the regressions approach, but the fitting process, coefficients, input data, etc. are very different, just look at the size of the input files.

Anyway, focus on the new model, if the new approach is not regression, the existing code might be irrelevant,

@jamesturner246 jamesturner246 closed this as not planned Won't fix, can't repro, duplicate, stale Nov 10, 2023
@github-project-automation github-project-automation bot moved this from 📋 Backlog to ✅ Done in Health-GPS Nov 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

No branches or pull requests

2 participants