Skip to content

Use the term 'relevant' instead of 'significant' in try run results #972

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
Aug 16, 2021

Conversation

rylev
Copy link
Member

@rylev rylev commented Aug 16, 2021

Fixes #968

This changes the try run result in two ways:

  • Uses the term "relevant" instead of "significant". Relevant better captures what the results are: results that we should care about in someway. "Significant" is a bit too overloaded. It can also mean "large" and "statistically significant" which is not the criteria we use for when to show changes in try run results.
  • It also adds the magnitude of the largest change to the message to give the reader an idea of how severe the changes are.

The changes in #968 would produce the following message under these changes:
"This change led to small relevant regressions 😿 in compiler performance."

@rylev rylev requested a review from Mark-Simulacrum August 16, 2021 09:37
@rylev rylev force-pushed the meaningful-not-signficiant branch from 857673a to bdcfe3e Compare August 16, 2021 09:37
@Mark-Simulacrum
Copy link
Member

"This change led to small relevant regressions crying_cat_face in compiler performance."

It might be worth a link or some connector here in the future -- e.g., "This change led to small, but still relevant, regressions..." I think relevant is also "interesting" in that there's an open question of 'relevant to whom?', in some sense. I think having a paragraph for the current rationale linked from the relevant word that talks about our method for determining what is relevant and how precisely authors/reviewers should judge those results would be good. For example, it is likely useful to discuss there that small wall time changes are still relevant and shouldn't be dismissed out of hand simply due to their low magnitude (but rather, for example, lack of impact on non-stress-test benchmarks, or some other reasoning).

@rylev rylev merged commit cb0d39b into rust-lang:master Aug 16, 2021
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.

Regression is described as both 'significant' and 'small'
2 participants