Skip to content

Revise SVR dataset scope and address uncaught errors#208

Open
ethanglaser wants to merge 5 commits intoIntelPython:mainfrom
ethanglaser:dev/eglaser-genreport-fixes
Open

Revise SVR dataset scope and address uncaught errors#208
ethanglaser wants to merge 5 commits intoIntelPython:mainfrom
ethanglaser:dev/eglaser-genreport-fixes

Conversation

@ethanglaser
Copy link
Copy Markdown
Contributor

@ethanglaser ethanglaser commented Apr 13, 2026

Description

CI status: http://intel-ci.intel.com/f13777e8-38d3-f195-8d41-a4bf010d0e2d


Checklist:

Completeness and readability

  • I have commented my code, particularly in hard-to-understand areas.
  • I have updated the documentation to reflect the changes or created a separate PR with updates and provided its number in the description, if necessary.
  • Git commit message contains an appropriate signed-off-by string (see CONTRIBUTING.md for details).
  • I have resolved any merge conflicts that might occur with the base branch.

Testing

  • I have run it locally and tested the changes extensively.
  • All CI jobs are green or I have provided justification why they aren't.
  • I have extended testing suite if new functionality was introduced in this PR.

@david-cortes-intel
Copy link
Copy Markdown
Contributor

@avolkov-intel I recall you also wanted to make changes like these - would this PR create some conflict later on?

"1st run time[ms]": res["time[ms]"][0],
"1st-mean run ratio": res["time[ms]"][0] / mean,
"time[ms]": mean,
"time CV": std / mean, # Coefficient of Variation
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It's very confusing to have a measurement called "time CV" reflecting the coefficient of variation when we also have procedures doing cross validation there. Also note that this is not actually the coefficient of variation due to the "boxed" methodology.

Perhaps could output just the standard deviation and name it 'std[ms]'. Or otherwise maybe could name it "std to mean (ratio)".

Copy link
Copy Markdown
Contributor Author

@ethanglaser ethanglaser Apr 14, 2026

Choose a reason for hiding this comment

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

I don't disagree with you, in fact for the large-scale branch that we have for multi-gpu we use std instead of CV. But I suggest revising this in a different PR as I did not change the logic here - the only reason the diff shows is because there was a condition added. Scope of this PR is resolving slowdowns/errors/failures and improving stability of CI jobs.

res.update(
{
"1st run time[ms]": res["time[ms]"][0],
"1st-mean run ratio": res["time[ms]"][0] / mean,
Copy link
Copy Markdown

@Vika-F Vika-F Apr 14, 2026

Choose a reason for hiding this comment

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

Is it possible to have mean == 0 here?

Suggested change
"1st-mean run ratio": res["time[ms]"][0] / mean,
"1st-mean run ratio": res["time[ms]"][0] / mean if mean != 0 else res["time[ms]"][0],

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

If the mean runtime is 0 then we have larger problems than a divide by zero. But I did not update the logic here (see main branch), the diff only shows because I added an additional condition (if isinstance(res["time[ms]"], list):). There is no diff within this condition.

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.

3 participants