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

fix SummarizeValues to return last non-NaN value #848

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

spacefreak86
Copy link

@spacefreak86 spacefreak86 commented Nov 29, 2024

When SummarizeValues is called with last or current as the aggregation function, it returns the last value of the series, whether it is NaN or not. This causes legendValue to output "NaN" in the legend, even if there are non-NaN values the series.

This PR changes the behavior of SummarizeValues so that the last non-NaN value is returned.

@spacefreak86
Copy link
Author

Could anybody take a look at this please? It is not the fault of this PR that the DeepSource test fails, it does not change the complexity of the SummarizeValues function.

Civil
Civil previously approved these changes Dec 19, 2024
@Civil Civil enabled auto-merge December 19, 2024 10:55
auto-merge was automatically disabled December 19, 2024 19:58

Head branch was pushed to by a user without write access

@spacefreak86
Copy link
Author

spacefreak86 commented Dec 19, 2024

Thanks @Civil. Unfortunately, I messed up the average part of SummarizeValues when I removed the unnecessary return path. I fixed that now.
When I run the tests, some of them still fail, but they where already failing without my changes.

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.

2 participants