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.

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

Several minor benchmarking changes #3196

Closed
wants to merge 3 commits into from

Conversation

esantorella
Copy link
Contributor

Summary:
This diff

  • Changes some Pandas indexing to avoid a "setting with copy" warning
  • Corrects the docstring of BenchmarkTrialMetadata
  • Renames ModelLauncherProblem to ModelLauncherTestFunction, since it is a test function and not a problem

Differential Revision: D67419032

@facebook-github-bot facebook-github-bot added the CLA Signed Do not delete this pull request or issue due to inactivity. label Dec 18, 2024
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D67419032

esantorella added a commit to esantorella/Ax that referenced this pull request Dec 18, 2024
Summary:

This diff
* Changes some Pandas indexing to avoid a "setting with copy" warning
* Corrects the docstring of `BenchmarkTrialMetadata`
* Renames `ModelLauncherProblem` to `ModelLauncherTestFunction`, since it is a test function and not a problem

Differential Revision: D67419032
@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.85%. Comparing base (6099333) to head (ea495e0).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3196      +/-   ##
==========================================
- Coverage   95.86%   95.85%   -0.01%     
==========================================
  Files         510      510              
  Lines       51499    51520      +21     
==========================================
+ Hits        49368    49387      +19     
- Misses       2131     2133       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Summary:

**Context**: Currently, benchmark metric tests conflate whether the metadata consumed by `fetch_trial_data` has multiple time steps with whether the output data should be `MapData`, and don't explicitly address the separate cases of whether or not there is a `BackendSimulator` attached to the metadata. They are also not organized well. These issues will become more pressing when we start to allow `BenchmarkMetric` to consume data with multiple time steps and/or use a backend simulator

**This diff**:
* Organizes those unit tests better
* Adds some more checks

Reviewed By: Balandat

Differential Revision: D67281379
…er time (facebook#3184)

Summary:

**Context**: There are situations where a metric is not a MapMetric, but its value changes depending on time.

**This diff**:
Introduces `BenchmarkTimeVaryingMetric`, which is not a MapMetric, can consume data with multiple time steps, is available while running, and requires a backend simulator.

Reviewed By: Balandat

Differential Revision: D67224949
Summary:

This diff
* Changes some Pandas indexing to avoid a "setting with copy" warning
* Corrects the docstring of `BenchmarkTrialMetadata`
* Renames `ModelLauncherProblem` to `ModelLauncherTestFunction`, since it is a test function and not a problem

Reviewed By: Balandat

Differential Revision: D67419032
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D67419032

esantorella added a commit to esantorella/Ax that referenced this pull request Dec 19, 2024
Summary:

This diff
* Changes some Pandas indexing to avoid a "setting with copy" warning
* Corrects the docstring of `BenchmarkTrialMetadata`
* Renames `ModelLauncherProblem` to `ModelLauncherTestFunction`, since it is a test function and not a problem

Reviewed By: Balandat

Differential Revision: D67419032
@facebook-github-bot
Copy link
Contributor

This pull request has been merged in 96ee89f.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed Do not delete this pull request or issue due to inactivity. fb-exported Merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants