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

Deprecate GenerationStrategy.trials_as_df #3275

Closed
wants to merge 2 commits into from

Conversation

saitcakmak
Copy link
Contributor

Summary: Deprecates GS.trials_as_df in favor of the more informative Experiment.to_df.

Differential Revision: D68629386

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

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

@codecov-commenter
Copy link

codecov-commenter commented Jan 27, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.01%. Comparing base (124f6e6) to head (dc1f6bf).
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3275      +/-   ##
==========================================
- Coverage   96.01%   96.01%   -0.01%     
==========================================
  Files         512      512              
  Lines       51725    51711      -14     
==========================================
- Hits        49663    49649      -14     
  Misses       2062     2062              

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

Summary:

Introduces `Experiment.to_df`, which will replace `exp_to_df` and `GS.trials_as_df` to present a unified API for summarizing an experiment. The summary will still be available as an analysis card via the `Summary` analysis class.

How does this differ from existing `exp_to_df` and `GS.trials_as_df`:
- `GS.trials_as_df` is rather barebones, and primarily lists the arms, trial status and the node/model that generated the arm. It does not appear to have much usage, so we'll deprecate it.
- The default behavior of `exp_to_df` differs in a few fields. It does not list the node name or metadata, and has an `is_feasible` field (missing here) that is computed in a somewhat strange way (for a boolean field). It also exposes a number of kwargs that can modify its behavior. These seem to be utilized in some plotting related utilities. The plan is to deprecate it in favor of `Experiment.to_df` after figuring out whether the additional options are necessary. If we want to expose an `is_feasible` field, we should come up with a better definition for the field that we utilize consistently across the codebase, such as in best point utilities.

Reviewed By: mgarrard, lena-kashtelyan

Differential Revision: D68720999
Summary:

Deprecates `GS.trials_as_df` in favor of the more informative `Experiment.to_df`.

Reviewed By: lena-kashtelyan

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

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

@facebook-github-bot
Copy link
Contributor

This pull request has been merged in 4bead35.

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