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

remove unnecessary data validation for trial index #3433

Closed
wants to merge 5 commits into from

Conversation

mgarrard
Copy link
Contributor

Summary:
previously in this diff stack we realized that this check is actually not that helpful, we remove it here.

Frankly, I don't fully understand how trial_index is being leveraged so could be helpful to get some UI ptrs for that

Differential Revision: D70301660

@facebook-github-bot facebook-github-bot added CLA Signed Do not delete this pull request or issue due to inactivity. fb-exported labels Feb 27, 2025
@facebook-github-bot
Copy link
Contributor

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

@codecov-commenter
Copy link

codecov-commenter commented Feb 27, 2025

Codecov Report

Attention: Patch coverage is 97.29730% with 1 line in your changes missing coverage. Please review.

Project coverage is 95.99%. Comparing base (92b9279) to head (de94c9d).

Files with missing lines Patch % Lines
ax/analysis/plotly/cross_validation.py 95.00% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main    #3433   +/-   ##
=======================================
  Coverage   95.99%   95.99%           
=======================================
  Files         539      539           
  Lines       52850    52860   +10     
=======================================
+ Hits        50735    50745   +10     
  Misses       2115     2115           

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

Mia Garrard added 4 commits February 27, 2025 10:52
…hing breaks if we update analysis_Base, Temporary Commit at 2/25/2025, 1:16:09 PM

Summary: ....

Differential Revision: D69725960
…ked changes)

Differential Revision: D69754828
Summary:
In the previous diff we exposed an adhoc comput method for CV, but the delta between the previous plot and our new plot is a degragation in UX. This diff fixes that by:
- tightening the autozoom
- making the points more transparent so they are more visible individually
- improving the hover
- adding x and y axis titles

Thanks for pointing some of these out Sam!

Differential Revision: D70195847
Summary:
This is the second ux improvement diff for adhoc cross validation computation. This allows for mulitple metrics to be computed at one time for a single adapter.

It does not currently tile or drop down the metic names. This is a nice improvement we'd like to make, but it wasn't immediately clear how to go about this

Differential Revision: D70200056
mgarrard added a commit to mgarrard/Ax that referenced this pull request Feb 27, 2025
Summary:

previously in this diff stack we realized that this check is actually not that helpful, we remove it here.

Frankly, I don't fully understand how trial_index is being leveraged so could be helpful to get some UI ptrs for that

Differential Revision: D70301660
Summary:
Pull Request resolved: facebook#3433

previously in this diff stack we realized that this check is actually not that helpful, we remove it here.

Frankly, I don't fully understand how trial_index is being leveraged so could be helpful to get some UI ptrs for that

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

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

@facebook-github-bot
Copy link
Contributor

This pull request has been merged in 1b22d05.

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