Skip to content

Factor out the time aggregation function, stop special-casing utility. #234

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.

Sign up for GitHub

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

Jump to bottom
Open
hmgaudecker wants to merge 20 commits into
base: main
Choose a base branch
from
Open

Factor out the time aggregation function, stop special-casing utility. #234

hmgaudecker wants to merge 20 commits into from
+5,008 −1,999

Conversation

@hmgaudecker
Copy link
Member

@hmgaudecker hmgaudecker commented Feb 10, 2026 •
edited
Loading

hmgaudecker and others added 10 commits February 10, 2026 06:29
@hmgaudecker @claude
Replace internal_regime_params with flat kwargs via rename_arguments.
4aad70e 
Instead of wrapping each function to accept an internal_regime_params dict and
extracting parameters at call time, use dags.signature.rename_arguments() to
qualify parameter names with function prefixes (e.g., risk_aversion becomes
utility__risk_aversion). This makes the parameter flow explicit and removes
the internal_regime_params indirection from all function signatures.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@hmgaudecker @claude
Rename InternalRegimeParams to FlatRegimeParams and flat_params to fl…
1669e9d 
…at_regime_params.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@hmgaudecker @claude
Rename params_template to regime_params_template where type is Regime…
f0b5cbe 
…ParamsTemplate.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@hmgaudecker @claude
Rename internal_fixed_params to flat_regime_fixed_params; type func a…
66b31c4 
…s Callable in result.py.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@hmgaudecker
Use QNAME_DELIMITER throughout since we need it in more general circu…
44a7627 
…mstances than REGIME_SEPARATOR. Some small improvements.
@hmgaudecker
Use pyproject-fmt and update hooks.
4ff6b86
@hmgaudecker
Simplify.
c90ee02
@hmgaudecker
Fix MY model.
60927f1
@hmgaudecker
Improve naming.
b560602
@hmgaudecker
Remove unnecessary defaults.
aaeabe4
@hmgaudecker hmgaudecker changed the base branch from main to remove-internal_regime_params February 10, 2026 10:06
@hmgaudecker hmgaudecker force-pushed the factor-out-time-aggregation branch 5 times, most recently from 255217d to 88bc936 Compare February 10, 2026 10:31
@hmgaudecker @claude
Factor out time aggregation function H.
be2b079 
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@hmgaudecker hmgaudecker force-pushed the factor-out-time-aggregation branch from 88bc936 to be2b079 Compare February 10, 2026 11:12
@review-notebook-app
Copy link

review-notebook-app bot commented Feb 10, 2026

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.

Powered by ReviewNB

hmgaudecker
hmgaudecker commented Feb 10, 2026
View reviewed changes
Copy link
Member Author

@hmgaudecker hmgaudecker left a comment

Choose a reason for hiding this comment

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

Autoreview.

@hmgaudecker hmgaudecker requested a review from timmens February 10, 2026 11:49
@hmgaudecker hmgaudecker changed the title Factor out the time aggregation function. Factor out the time aggregation function, stop special-casing utility. Feb 10, 2026
@hmgaudecker
Copy link
Member Author

hmgaudecker commented Feb 10, 2026

@timmens:

  • The crucial changes to time aggregation are in if you want to have a look at them in isolation. Just be careful with Q_and_F, a chunk of code had accidentally slipped in here, which I removed in 2eb5b92 again.
  • 0f235c5 moves utility into the user functions. All those special cases had confused the hell out of me many times, this PR seemed like a good spot to do it since the structure is very similar to H. Apologies for the bloat.
  • Rest are just simplifications / clarifications.

@hmgaudecker hmgaudecker mentioned this pull request Feb 13, 2026
Merged
timmens
timmens approved these changes Feb 13, 2026
View reviewed changes
Copy link
Member

@timmens timmens left a comment

Choose a reason for hiding this comment

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

Very very nice PR! I like the change a lot.

My main comment is that I think we should rename H for the user. An example could be aggregator or time_aggregator. All other user-facing argument or attribute names are expressive and not related to the math notation, so I would not expect it here.

Why I added utility as its own attribute

While I do think that in the current state of PyLCM it makes sense to add utility and the time aggregator in the functions dictionary, here are some of my thoughts on why I pulled utility out, in case we want to do that in the future again:

  • Each attribute of the Regime class should correspond to an object that is related to the Economic problem. Utility is such an object, auxiliary functions are not.
  • Using the functions dictionary makes our model specifications much more readable, but it is not necessary in principle. A user could also just write a single utility function.
  • For objects like the time aggregator, typing is easier if it would be a Regime attribute.

Nevertheless, the fact that with this PR we can remove a lot of special cases, and the parameter processing works smoothly for all functions (also for the meta-level functions like the time aggregator) is the big reason why your implementation wins right now.

@hmgaudecker
Copy link
Member Author

hmgaudecker commented Feb 13, 2026

Agreed! I had that H -> time_aggregation thought at some point as well, especially after seeing the continuation_value argument.

The only thing that kept me was the thought that for the next level (truly fixing #174, including cases like hyperbolic discounting), we'll have to make a bunch of the Q_... functions user-facing/overridable, too.

I don't have a strong opinion -- what do you think?

Thanks for the explainer re utility!

Base automatically changed from remove-internal_regime_params to main February 13, 2026 12:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@timmens timmens timmens approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

ENH: Make aggregation step in Q_and_F more flexible

2 participants

@hmgaudecker @timmens