Skip to content

Conversation

@cmelchior
Copy link
Contributor

Two examples were added:

  • "Simple analysis": Plot data from a single benchmark run
  • "Compare analysis": Compare two runs of the same benchmark suite to find changes that are worth investigating further.

How these notebooks look can be seen here:

  1. Simple: https://gist.github.com/cmelchior/1b6007752333d1ce76a1d02ed311a135
  2. Compare: https://gist.github.com/cmelchior/76e87e5207a87c5c4968c31a8910a88a

Some questions for reviewers:

  1. Are the notebooks easy to follow?
  2. Are there too many comments? Too few?
  3. Should we merge some of the intermediate steps? Currently, I try to make it more explicit what happens, but this also makes the text more verbose.
  4. Do the math "checks out", i.e. is my way of using the data sensible or do you think something else would be more appropriate?

Two examples was added:
- "Simple analysis": Plot data from a single benchmark run
- "Compare analysis": Compare two runs of the same benchmark suite to find changes that are worth investigating more.
@koperagen
Copy link

koperagen commented Oct 10, 2025

val oldDf = oldRun.toDataFrame().insert("rowIndex") { index() }.at(0)
can be replaced with addId shortcut:
val oldDf = oldRun.toDataFrame().addId("rowIndex")

val mergedData = resultData.unfold { runOld }.unfold { runNew }.flatten()
i think two unfold operations can be merged like this:
val mergedData = resultData.unfold { runOld and runNew }.flatten()

You can create dataframe directly without intermediate classes if you want:

val resultData = combinedData.mapToFrame {
        "name"  from { it.benchmark }
        // same for other params
        "runOld" {
            "score" from { it.primaryMetric.score }
            "range" from { it.primaryMetric.scoreConfidence[0]..it.primaryMetric.scoreConfidence[1] }
        }
        // repeat for runNew
}
// benchmarks
group.value.toDataFrame { // same api as in mapToFrame }

@fzhinkin fzhinkin self-requested a review October 10, 2025 14:44
@fzhinkin fzhinkin self-assigned this Oct 10, 2025
@fzhinkin
Copy link
Collaborator

@cmelchior, thank you for creating notebooks! It looks good and the math looks correct. In terms of comments, everything is nice.
The only thing I would change is printing intermediate data frames (in paragraphs 5, 6, and 7) in the "Compare" notebook: they are indispensable if someone wants to check the result of transformation, but given how many date it outputs, it might be better to comment out their printing (like in the paragraph 3 of the "Simple" notebook) to abstain from stealing attention from final results.

@cmelchior
Copy link
Contributor Author

Great, thanks 👍

I updated the notebooks with your and @koperagen's comments.

The updated notebooks are here:

Copy link
Collaborator

@fzhinkin fzhinkin left a comment

Choose a reason for hiding this comment

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

@cmelchior, thank you! I'll update the readme and start suggesting notebooks as part of plugin's output in a follow-up PR.

@cmelchior cmelchior merged commit 6b14c81 into master Oct 24, 2025
1 check passed
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.

4 participants