Skip to content
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.

Sign up for GitHub

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

Jump to bottom

chore: unit test regressors to match estimator results #1804

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

chore: unit test regressors to match estimator results #1804

wants to merge 1 commit into from

Conversation

rootfs
Copy link
Contributor

@rootfs rootfs commented Oct 9, 2024

No description provided.

@rootfs rootfs requested review from sthaha and sunya-ch October 9, 2024 14:57
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Oct 9, 2024 •
edited
Loading

🤖 SeineSailor

Here is a concise summary of the pull request changes:

Summary: This pull request introduces unit tests for the logarithmic regression model and SGD regressor model in the regressor package, specifically testing the GetComponentsPower function and power calculation results. The changes are internal and do not affect the external interface or behavior of the code.

Key Modifications:

  • Added unit tests for logarithmic regression model and SGD regressor model
  • Tested GetComponentsPower function and power calculation results

Impact: The changes are limited to internal testing code and do not affect the external interface or behavior of the code.

Observations/Suggestions:

  • It's great to see the addition of unit tests to improve code coverage and reliability.
  • Consider adding more comprehensive tests to cover edge cases and ensure the models' correctness.
  • Since this is a work-in-progress change, it's essential to ensure that the tests are properly reviewed and validated before merging.

@rootfs rootfs changed the title [WIP] chore: unit test regressors to match estimator results chore: unit test regressors to match estimator results Oct 9, 2024
sunya-ch
sunya-ch approved these changes Nov 6, 2024
View reviewed changes
Copy link
Collaborator

@sunya-ch sunya-ch left a comment

Choose a reason for hiding this comment

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

Mostly looks good to me. Just leave a comment.
Thank you.

pkg/model/estimator/local/regressor/local_test.go
// Test power calculation. The results should match those from estimator
// https://github.com/sustainable-computing-io/kepler-model-server/pull/493#discussion_r1795610556
testModel("https://raw.githubusercontent.com/sustainable-computing-io/kepler-model-db/refs/heads/main/models/v0.7/ec2-0.7.11/rapl-sysfs/AbsPower/BPFOnly/SGDRegressorTrainer_0.json",
types.LinearRegressionTrainer, 146994, 18704, 146994)
Copy link
Collaborator

Choose a reason for hiding this comment

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

We may make the note that the expected value must be updated if when the model at the target URL has changed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sunya-ch sunya-ch sunya-ch approved these changes

@sthaha sthaha Awaiting requested review from sthaha

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@rootfs @sunya-ch