Skip to content

Statistics: add scaling functionality #955

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

Merged
merged 1 commit into from
Apr 15, 2025

Conversation

mgovers
Copy link
Member

@mgovers mgovers commented Apr 14, 2025

This is a cherry-pick of the statistics functionality in #951 .

It adds statistics::scale functions next to the already present statistics::combine functions

Do not merge before #947 and #954 are merged

@mgovers mgovers added the improvement Improvement on internal implementation label Apr 14, 2025
@mgovers mgovers self-assigned this Apr 14, 2025
@mgovers mgovers force-pushed the feature/add-scaling-to-statistics branch from 4820105 to a71a4ee Compare April 14, 2025 14:11
@mgovers mgovers added the do-not-merge This should not be merged label Apr 14, 2025
Base automatically changed from feature/current-sensor-measurements-observability to main April 15, 2025 08:18
@figueroa1395
Copy link
Contributor

These set of scale function, and their tests, were taken literally from #951, right?

One minor nitpick, could you add an in-code comment above the scale functions implementation with the permalink to the issue where this calculations are done, just for reference. Or better yet, also shortly add in-code from where you start and where you get in the variance scale specifically? Just so that we see where does the calculations come from once we have forgotten about this.

Oh, I will approve after possible merge conflicts (as stated in the introduction of the PR) are resolved.

@mgovers mgovers removed the do-not-merge This should not be merged label Apr 15, 2025
@mgovers
Copy link
Member Author

mgovers commented Apr 15, 2025

These set of scale function, and their tests, were taken literally from #951, right?

yes

One minor nitpick, could you add an in-code comment above the scale functions implementation with the permalink to the issue where this calculations are done, just for reference. Or better yet, also shortly add in-code from where you start and where you get in the variance scale specifically? Just so that we see where does the calculations come from once we have forgotten about this.

what do you mean?

Oh, I will approve after possible merge conflicts (as stated in the introduction of the PR) are resolved.

no merge conflicts because i had already resolved those

@figueroa1395
Copy link
Contributor

what do you mean?

As in, as taken from #547, just add:

$$\begin{align} var = \sqrt{\sigma_r^2 + \sigma_i^2} \end{align}$$

but for the actual variance scaling, above is just for demonstration of what I mean. So add an in-code comment with the formula.

no merge conflicts because i had already resolved those

Yup, realized after commenting.

@mgovers mgovers changed the base branch from main to feature/current-sensor-math-solver April 15, 2025 10:42
@mgovers mgovers added the do-not-merge This should not be merged label Apr 15, 2025
figueroa1395
figueroa1395 previously approved these changes Apr 15, 2025
@mgovers mgovers force-pushed the feature/add-scaling-to-statistics branch from a71a4ee to ecf4e54 Compare April 15, 2025 11:20
@mgovers mgovers removed the do-not-merge This should not be merged label Apr 15, 2025
Base automatically changed from feature/current-sensor-math-solver to main April 15, 2025 11:41
@mgovers mgovers dismissed figueroa1395’s stale review April 15, 2025 11:41

The base branch was changed.

@mgovers mgovers force-pushed the feature/add-scaling-to-statistics branch from ca3749c to 850e013 Compare April 15, 2025 11:58
Signed-off-by: Martijn Govers <Martijn.Govers@Alliander.com>
@mgovers mgovers force-pushed the feature/add-scaling-to-statistics branch from 850e013 to a206d17 Compare April 15, 2025 12:00
@mgovers mgovers enabled auto-merge April 15, 2025 12:08
Copy link

@mgovers mgovers added this pull request to the merge queue Apr 15, 2025
Merged via the queue into main with commit 0a529c1 Apr 15, 2025
30 checks passed
@mgovers mgovers deleted the feature/add-scaling-to-statistics branch April 15, 2025 14:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
improvement Improvement on internal implementation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants