Skip to content

fix integral and min max preservation#282

Open
l-kotzur wants to merge 1 commit intodevelopfrom
integral_minmax_preservation
Open

fix integral and min max preservation#282
l-kotzur wants to merge 1 commit intodevelopfrom
integral_minmax_preservation

Conversation

@l-kotzur
Copy link
Copy Markdown
Collaborator

Description

In rare cases tsam violates the original lower or upper bounds of the time series leading to a UserWarning: At least one maximal value of the aggregated time series exceeds the maximal value the input time series for: ...

Motivation and Context

This fixes the scaling and the min max distribution representation.

Type of Change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation update

Checklist

  • My code follows the project's code style (run ruff check and ruff format)
  • I have added tests that prove my fix/feature works
  • All new and existing tests pass (pytest test/)
  • I have updated the documentation if needed

@FBumann
Copy link
Copy Markdown
Collaborator

FBumann commented Apr 21, 2026

@l-kotzur I really like this. But i think we need to regenerate the golde test files for this to pass ci.
Also, it seems like this could hurt performance due to the 20 iterations.
Furthermore, would you mind adding this after #234 is merged? It seems to me that such changes will be much more easy to implement there, and each change while #234 sits waiting leads to extra work there.

But i fully understand if you want to merge this before of course :)

@FBumann FBumann added the bug label Apr 21, 2026
@julian-belina
Copy link
Copy Markdown
Collaborator

@l-kotzur, thank you very much for looking into this. I hope I can review and merge it by the end of the week.

@l-kotzur
Copy link
Copy Markdown
Collaborator Author

Thanks, I have no hurries. Do the #234 first. Then we can remove the golde test and discuss performance.

@FBumann
Copy link
Copy Markdown
Collaborator

FBumann commented Apr 21, 2026

@l-kotzur just to make sure:
The golden regression tests are there to stay! They wont get removed.
In cases like this where a bug is fixed that leads to different results, we need to simply decide:

  1. Fix the bug by updating the golden regression files and merge the bugfix. THis will need proper release notes, as results of users will differ
  2. Defer the bugfix, add a config option to opt in or do sth else to make it non-breaking

Option 1 should be prefered if the differences are as small as in this case, and if its an actual bug...

Option 2 adds complexity and should only be chosen for good reasons

@julian-belina julian-belina self-assigned this Apr 30, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants