Skip to content

Add aggregate_time support to convert_and_aggregate#491

Merged
FabianHofmann merged 4 commits intomasterfrom
feat/aggregate_time
Mar 25, 2026
Merged

Add aggregate_time support to convert_and_aggregate#491
FabianHofmann merged 4 commits intomasterfrom
feat/aggregate_time

Conversation

@FabianHofmann
Copy link
Copy Markdown
Contributor

@FabianHofmann FabianHofmann commented Mar 13, 2026

Closes #489.

Changes proposed in this Pull Request

  • add aggregate_time={"sum", "mean", None} to convert_and_aggregate to control temporal aggregation explicitly
  • default to aggregate_time="legacy", which preserves historical context-dependent behavior (sum without spatial agg, full timeseries with spatial agg) and emits a FutureWarning nudging users to pick an explicit option
  • None means "no temporal aggregation"
  • deprecate capacity_factor and capacity_factor_timeseries in favor of aggregate_time
  • extract _aggregate_time() helper for consistent temporal aggregation with keep_attrs=True in both code paths
  • update wind() and pv() examples to use aggregate_time=None
  • add focused tests with shared fixtures for temporal aggregation, deprecated arguments, and invalid argument validation

Checklist

  • Code changes are sufficiently documented; i.e. new functions contain docstrings and further explanations may be given in doc.
  • Unit tests for new features were added (if applicable).
  • Newly introduced dependencies are added to environment.yaml, environment_docs.yaml and setup.py (if applicable).
  • A note for the release notes doc/release_notes.rst of the upcoming release is included.
  • I consent to the release of this PR's code under the MIT license.

Copy link
Copy Markdown
Contributor

@brynpickering brynpickering left a comment

Choose a reason for hiding this comment

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

Definitely needed - all my students have been confused by the existing arguments when first working with atlite. My comments are mostly for maintainability but the choice of aggregate_time options for backwards-compatibility might need discussion.

Comment thread atlite/convert.py Outdated
Comment thread atlite/convert.py Outdated
Comment thread atlite/convert.py Outdated
Comment thread atlite/convert.py Outdated
Comment thread atlite/convert.py Outdated
Comment thread test/test_aggregate_time.py Outdated
Comment thread test/test_aggregate_time.py Outdated
Comment thread test/test_aggregate_time.py Outdated
Comment thread test/test_aggregate_time.py Outdated
Comment thread atlite/convert.py Outdated
…te_time helper, consistent keep_attrs, use fixtures in tests
@FabianHofmann
Copy link
Copy Markdown
Contributor Author

Thanks for the review @brynpickering! I've addressed all your comments:

  • aggregate_time now defaults to "legacy", which preserves the old context-dependent behavior but emits a FutureWarning telling users to switch to an explicit value. Next release we can remove it and default to something explicit (probably "sum"). Replaced False with None.
  • extracted the temporal aggregation into a small helper. This also fixes the keep_attrs inconsistency — both "sum" and "mean" now use keep_attrs=True.
  • layout and result_ts are now module-level fixtures.

Copy link
Copy Markdown
Contributor

@brynpickering brynpickering left a comment

Choose a reason for hiding this comment

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

This makes more sense to me now. The option one needs to give to ensure backwards-compatibility is much clearer. The next feature release, I would raise an exception on use of capacity_factor and capacity_factor_timeseries but possibly keep legacy, only removing legacy on the subsequent feature release.

Side note: should the use of capacity_factor=True in example notebooks already be converted to using aggregate_time?

@FabianHofmann
Copy link
Copy Markdown
Contributor Author

Side note: should the use of capacity_factor=True in example notebooks already be converted to using aggregate_time?

good point, will do that

@FabianHofmann FabianHofmann merged commit 44cd694 into master Mar 25, 2026
19 checks passed
@FabianHofmann FabianHofmann deleted the feat/aggregate_time branch March 25, 2026 16:30
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.

Convert & Aggregate redesign

2 participants