Skip to content

Refactor Modifiers#570

Draft
TimothyWillard wants to merge 12 commits intodevfrom
feature/409/refactor-modifiers
Draft

Refactor Modifiers#570
TimothyWillard wants to merge 12 commits intodevfrom
feature/409/refactor-modifiers

Conversation

@TimothyWillard
Copy link
Contributor

@TimothyWillard TimothyWillard commented Jun 2, 2025

Describe your changes.

  • Add pydantic representations for distributions with a sample method for usage here. Ideally would also be used in likelihood, but should reserve this for a follow up issue+PR.
  • Add Gamma, Beta, and Weibull distributions, [Feature request]: Gamma and Weibull Distribution support #573.
  • Add unit tests for existing gempyor.NPI functionality, to build a baseline of existing behavior as needed.
  • nb: moving from "reduce" to "apply modifiers"
  • Add apply abstract method to ModifierABC taking arguments parameter_name, parameter, method and using SinglePeriodModifier as an example implement reduce. nb: see helpers.py for ex. Should drop into:
    def parameters_reduce(self, p_draw: ndarray, npi: object) -> ndarray:
    """
    Params reduced according to the NPI provided.
    Args:
    p_draw: A numpy array of shape (`npar`, `n_days`, `nsubpops`) like that
    returned by `parameters_quick_draw`.
    npi: An NPI object describing the parameter reduction to perform.
    Returns:
    An array the same shape as `p_draw` with the prescribed reductions
    performed.
    """
    p_reduced = copy.deepcopy(p_draw)
    if npi is not None:
    for idx, pn in enumerate(self.pnames):
    npi_val = NPI.reduce_parameter(
    parameter=p_draw[idx],
    modification=npi.getReduction(pn.lower()),
    method=self.pdata[pn]["stacked_modifier_method"],
    )
    p_reduced[idx] = npi_val
    if "rolling_mean_windows" in self.pdata[pn]:
    p_reduced[idx] = utils.rolling_mean_pad(
    data=npi_val, window=self.pdata[pn]["rolling_mean_windows"]
    )
    return p_reduced
    .
  • Add apply_dataframe method to ModifierABC that is a wrapper around apply.
  • Add ModifiersCollection using pydantic.BaseModel to represent the 'outcome_modifiers' and 'seir_modifiers' sections in config files. Pretty minimal, scenarios: list[str] and modifiers: list[Modifier].
  • Refactor ModifiersCollection to have a scenarios_map attribute (needs better name or maybe some rethinking, ideally would be called scenarios but that attribute is already occupied) that is dict[str, list[str]] to represent "stacked modifiers". Requires adding a model validator to ModifiersCollection to rewrite StackedModifiers to this to maintain backwards compatibility.
  • Add reduce (now .apply()) method to ModifiersCollection that implements similar logic to the current StackedModifier that takes a scenario and modifies multiple parameters at once.
  • Implement MultiPeriodModifier. (ie, incorporate MultiPeriodModifier into PeriodicModifier)
  • Address [Bug]: selected option for subpop_setup fails if modifiers apply to only a single subpop #490 if not already addressed by stricter validation (might require following up for details/cmds).
  • Integrate ModifiersCollection into gempyor.seir/outcomes.
  • Explore possibilities for Refactor Modifiers #570 (comment). Doable with some meta-programming, but loose the ability to validate inputs at configuration parse time.
  • Ensure renaming from "reduce" to "apply modifiers"
  • git rm the NPI subpackage.

Does this pull request make any user interface changes? If so please describe.

The user interface changes are...

Those are reflected in updates to the documentation in ...

What does your pull request address? Tag relevant issues.

This pull request addresses: #409.

Copy link
Contributor

@pearsonca pearsonca left a comment

Choose a reason for hiding this comment

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

Just some thoughts on the work in progress

Copy link
Contributor

Choose a reason for hiding this comment

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

This seems like a good start for a roll-our-own approach to distributions.

But: how much of an overhaul elsewhere would it be to just use https://www.pymc.io/projects/docs/en/stable/api/distributions.html (for example, there are other libraries that also manage this kind of thing)

Copy link
Contributor

Choose a reason for hiding this comment

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

It already seems like mostly what's happening is wrapping rng.SOMENAME - there is a way to have that as the default in the parent class (basically, use distribution field to know what function to call, have kwargs built from internal fields) and then override for the cases that really need to be custom (e.g. truncnorm)?

@jcblemai
Copy link
Collaborator

jcblemai commented Jul 4, 2025

I could not really read the diff which is huge, and I don't follow much flepi but my 2-cent is that I would ditch the recursive autocall modifier thing, which makes anything like smart inference hard. I would load them all at once from config.

config breaking change I think are necessary:

  • Every modifier is case of multi-timereduce so I would just keep this class to ease up the syntax. Probably something can be made to keep config for functioning
  • I would probably ditched the stacked-modifier (not now perhaps) because it is confusing. But this is up to debate

emprzy added a commit that referenced this pull request Jul 9, 2025
These tests are duplicates, which gets addressed in #570 , but for now we need to add both to the slow group to allow workflow success.
@TimothyWillard TimothyWillard force-pushed the feature/409/refactor-modifiers branch 2 times, most recently from 7bb6035 to 9dc7923 Compare July 9, 2025 20:08
@TimothyWillard TimothyWillard removed their assignment Jul 22, 2025
emprzy added 12 commits August 1, 2025 10:11
There was a doubly-named test in `test_npis.py` that also contained an outdated loc .
Implement this method in `_single_period::SinglePeriodModifier` class.
`.apply_dataframe()` is a wrapper around `.apply()`
Additionally, rename `SinglePeriodModifier` to `PeriodicModifier` (now represents both single and mulitperiod)
Implement this method in `_single_period::SinglePeriodModifier` class.
`.apply_dataframe()` is a wrapper around `.apply()`
Additionally, rename `SinglePeriodModifier` to `PeriodicModifier` (now represents both single and mulitperiod)
@emprzy emprzy force-pushed the feature/409/refactor-modifiers branch from 8f7e50f to 702d108 Compare August 1, 2025 14:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Feature request]: Refactor gempyor.NPI Into gempyor.modifiers

4 participants