Skip to content

feat: add ~variance~ standard deviation selector#22

Open
michaelpun wants to merge 11 commits intomainfrom
mol-2751-feat-implement-variance-selector
Open

feat: add ~variance~ standard deviation selector#22
michaelpun wants to merge 11 commits intomainfrom
mol-2751-feat-implement-variance-selector

Conversation

@michaelpun
Copy link
Contributor

This PR adds VarianceSelector class which straightforwardly returns the variance of a metric.

From the Selector base class, I think this is within the spirit of the class to return an aggregation of the metric but due to the name "Selector" I wonder if this goes against the spirit due to it no longer having the same units as the original metric.

@michaelpun michaelpun requested a review from padix-key April 30, 2025 19:47
Copy link
Collaborator

@padix-key padix-key left a comment

Choose a reason for hiding this comment

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

Thanks for the feature. This is a more exotic use case, but I still think this can be useful for a broader audience and probably an implementation as Selector makes the most sense.

However, I also share your concern that we are not actually 'selecting' a value. A possible solution to this would be to change the name to VarianceAggregation or something similar.
To make the VarianceSelector use the same units as its input, one could also think about returning the standard deviation instead of the variance, if this still fulfills the purpose.

michaelpun and others added 2 commits May 7, 2025 16:27
Co-authored-by: Patrick Kunzmann <padix.key@gmail.com>
@michaelpun michaelpun force-pushed the mol-2751-feat-implement-variance-selector branch from 94613fd to 9ec2f8a Compare May 7, 2025 23:33
@michaelpun michaelpun force-pushed the mol-2751-feat-implement-variance-selector branch from 9ec2f8a to 38e6b4f Compare May 7, 2025 23:35
@michaelpun
Copy link
Contributor Author

Thanks for the feedback!

To make the VarianceSelector use the same units as its input, one could also think about returning the standard deviation instead of the variance, if this still fulfills the purpose.

I think this is a great suggestion. After using the VarianceSelector for a bit, I think having the same units would be good both for the code and for the ease of interpretability on the user's end. I've replaced VarianceSelector with StandardDeviationSelector in 38e6b4f. Let me know if the name is too long.

However, I also share your concern that we are not actually 'selecting' a value. A possible solution to this would be to change the name to VarianceAggregation or something similar.

I think this could make sense. If I were to rename the now-called StandardDeviationSelector to StandardDeviationAggregator would it make sense to also rename MeanSelector since the value it produces is not a member of the set of outputted values? Or would the "selector" distinction just be for metrics that are directly tied to the random variable and "aggregator" just be for higher order statistics and other aggregations?

@michaelpun michaelpun changed the title feat: add variance selector feat: add ~variance~ standard deviation selector May 7, 2025
@padix-key
Copy link
Collaborator

Let me know if the name is too long.

Would DeviationAggregator also suffice?

Or would the "selector" distinction just be for metrics that are directly tied to the random variable and "aggregator" just be for higher order statistics and other aggregations?

I would say so. But maybe a third opinion would be good here. @danielkovtun @emalgorithm @dtischer Do you prefer StandardDeviationAggregator, DeviationAggregator, StandardDeviationSelector or DeviationSelector?

@aivant aivant deleted a comment from linear bot Jan 15, 2026
@michaelpun michaelpun requested a review from padix-key January 16, 2026 18:59
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