Skip to content

Add API to remove columns for baseline comparison #299

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

Closed
Drawaes opened this issue Nov 15, 2016 · 11 comments · Fixed by #1890
Closed

Add API to remove columns for baseline comparison #299

Drawaes opened this issue Nov 15, 2016 · 11 comments · Fixed by #1890

Comments

@Drawaes
Copy link

Drawaes commented Nov 15, 2016

Most of the time I don't want to compare sd and other info against the baseline.

@AndreyAkinshin
Copy link
Member

Ok, let's discuss what we can do here. I have several thoughts.

Baseline*Column attributes

Different people want to have different columns when they are working with baselines. We have 3 main categories: Scaled (A/B), DiffAbsolute (A-B), DiffRelative ((A-B)/B*100%). Each category could have a corresponded StdDev column which could be optional (we will show this column only when StdDev is too big) or mandatory. Also, we should define a threshold for StdDev (a column will be shown only if there is at least one benchmark with StdDev>threshold). Examples:

// Style 1
[BaselineColumn] // scaled values
[BaselineColumn(BaselineColumnKind.DiffAbsolute)]
[BaselineColumn(BaselineColumnKind.DiffRelative)]

[BaselineStdDevColumn] // mandatory column
[BaselineStdDevColumn(threshold: 0.05)] // will be shown if we have values < 0.95 or > 1.05
[BaselineStdDevColumn(BaselineColumnKind.DiffRelative)]
[BaselineStdDevColumn(BaselineColumnKind.DiffRelative, threshold: 0.05)]

// Style 2
[BaselineScaledColumn]
[BaselineDiffAbsoluteColumn]
[BaselineDiffRelativeColumn]
[BaselineScaledStdDevColumn]
[BaselineDiffRelativeStdDevColumn(threshold: 0.05)]

// Style 3
[BaselineColumn] // scaled with optional StdDev
[BaselineColumn(stdDevThreshold: 0.05)] // scaled with optional StdDev
[BaselineColumn(stdDevThreshold: 0)] // scaled with mandatory StdDev
[BaselineColumn(stdDevThreshold: double.NaN)] // scaled with disabled StdDev
[BaselineColumn(BaselineColumnKind.DiffRelative, stdDevThreshold: 0.05)]

Which of these styles (or some combination of them) should be implemented?
If there are no specified baseline columns, the default will be used (scaled + optional StdDev with some threshold).

Analysers for baseline StdDev columns

Some users could be confused if the magical StdDev columns will be printed only for part of all summary tables without any explanation. Thus, analysers which prints messages like "Hey, StdDev for scaled values is too big, be careful" should be implemented. And this analyzer should be active even for benchmarks with disabled StdDev column. I have some reasons for that: often people look at Scaled = 1.05 and think something like "This benchmark shows that method X is 5% faster than method Y". However, StdDev for the scaled values could be 20%; X and Y could don't have any significant difference in performance. We definitely should warn a user in this case. But if StdDev is small for all jobs, we can easily skip it (if it wasn't required manually).
Does it make any sense?

Other columns

We have a big challenge in the summary table: it should be as small as possible but it should include all important information. Thus, some of the columns are optional; they will be printed only if BenchmarkDotNet consider them as important columns. For example, MedianColumn will be printed only if we have a job with Median which is outside of 99% confidence interval. StdError will be printed only if it is bigger than Mean * 0.01. And so on. I believe that we will have many optional default columns in the future. Now we should think about common API for the following user requests:

  • I understand that this column could be important but I still want to disable it for my summary.
  • I understand that this column is important but I still override the "print conditions"
  • I want always to have this column.

I suggest using the following approach. Let's define an enum:

public enum ColumnPolicy {
    Disabled, Optional, Mandatory
}

Each column will have such property and it could be specified via attribute. An explicitly defined column overrides the default column. For example, we could have optional median column, but it will be always disabled, if we write

[MedianColumn(policy: ColumnPolicy.Disabled)]
// or
[MedianColumn(ColumnPolicy.Disabled)]

We could also override "print conditions" in such attributes:

[MedianColumn(ColumnPolicy.Optional, ConfidenceLevel.L99)]

Discussion

I believe that that it's a right direction for development, but I'm not sure which API should be chosen. It's really hard to predict which style will be simple and clear. So, any ideas are welcome.

/cc @adamsitnik, @mattwarren, @Drawaes, @benaadams

@adamsitnik
Copy link
Member

@AndreyAkinshin An idea:

extend IColumnProvider with IEnumerable<IColumn> Exclude()

then if somebody does not want to see some column he/she can create an exluding column provider with something similar to:

class ExcludingColumns : IColumnProvider
{
    public IEnumerable<IColumn> Exclude()
    {
        yield return BaselineScaledColumn.Scaled;
    }

    public IEnumerable<IColumn> GetColumns(Summary summary) => Enumerable.Empty<IColumn>();
}

and register it in their own Config

@AndreyAkinshin
Copy link
Member

@adamsitnik, sounds interesting, but I also want to have an "attribute" way to exclude columns. Any ideas how to do it?

@adamsitnik
Copy link
Member

hmm

due to An attribute argument must be a constant expression, typeof expression or array creation expression of an attribute parameter type we can't create an attribute that would accept params IColumn[] excluded

My only idea for now it to add new enum ColumnType, extend IColumn with getter which would return in and then create a new attribute that would accept this enum. If we make it flag then we could make sth like:

[Exclude(ColumnType.BaselineScaled | ColumnType.BaselineScaledStdDev)]

the disadvantage is that it would be limitted to columns implemented inside BenchmarkDotNet, so people could not disable their custom columns this way. But on the other hand if they have enabled it they can also disable it by not using their custom ColumnProvider.

@AndreyAkinshin
Copy link
Member

@adamsitnik, what do you think about my approach with ColumnPolicy:

[MedianColumn(policy: ColumnPolicy.Disabled)]

This, columns attribute do not only introduce a column but also specify (or override) policy (Disabled, Optional, Mandatory) for this column.

@adamsitnik
Copy link
Member

what do you think about my approach with ColumnPolicy

sorry @AndreyAkinshin I missed that. Good idea! Especially that we have an attribute for each column now!

@AndreyAkinshin AndreyAkinshin modified the milestones: v0.10.2, v0.10.1 Dec 2, 2016
@AndreyAkinshin
Copy link
Member

Another idea: we could also override names of the columns for cases when user want to use a standard column with another name. Examples: RPS instead of Op/s (see also: https://github.com/aspnet/KestrelHttpServer/blob/2016578f4eba0dcd243f47a47410962f39f5d97e/test/Microsoft.AspNetCore.Server.Kestrel.Performance/columns/RpsColumn.cs), Avr instead of Mean, and so on.

@adamsitnik
Copy link
Member

@AndreyAkinshin Nice feature, but most probably only our super users would use. Will it be easy to implement?

@AndreyAkinshin
Copy link
Member

Yep, I will do it as a part of column refactoring (v0.10.2).

@AndreyAkinshin AndreyAkinshin modified the milestones: v0.10.3, v0.10.2 Jan 14, 2017
@AndreyAkinshin AndreyAkinshin modified the milestones: v0.10.x, v0.10.3 Jan 27, 2017
@adamsitnik
Copy link
Member

@AndreyAkinshin was it implemented? I am not sure?

@AndreyAkinshin
Copy link
Member

@adamsitnik, no.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants