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

Ordering trial by value should always de-prioritise undefined values #666

Merged
merged 3 commits into from
Nov 1, 2023

Conversation

adjeiv
Copy link
Contributor

@adjeiv adjeiv commented Oct 21, 2023

Contributor License Agreement

This repository (optuna-dashboard) and Goptuna share common code.
This pull request may therefore be ported to Goptuna.
Make sure that you understand the consequences concerning licenses and check the box below if you accept the term before creating this pull request.

  • I agree this patch may be ported to Goptuna by other Goptuna contributors.

Reference Issues/PRs

Fixes #264

What does this implement/fix? Explain your changes.

As we aren't considering the ascending/descending nature of sort, we end up having trials with no value appearing above trials with values in certain cases (see referenced issue). This fix takes this into account.

chrome_kHA0xB5rbc chrome_xT0n2Qf8bw

@codecov
Copy link

codecov bot commented Oct 21, 2023

Codecov Report

Merging #666 (31360a4) into main (14afdee) will increase coverage by 0.23%.
Report is 13 commits behind head on main.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main     #666      +/-   ##
==========================================
+ Coverage   62.65%   62.88%   +0.23%     
==========================================
  Files          35       35              
  Lines        2252     2250       -2     
==========================================
+ Hits         1411     1415       +4     
+ Misses        841      835       -6     

see 1 file with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@contramundum53
Copy link
Member

contramundum53 commented Oct 24, 2023

Could you fix the linter error? When it's fixed, a reviewer will be assigned.

@adjeiv
Copy link
Contributor Author

adjeiv commented Oct 25, 2023

I've suppressed the errors for now, as we don't seem to have any variable pattern exclusions for this rule. Separately, not sure why that hasn't fixed the check.

@contramundum53
Copy link
Member

Could you run

npm run fmt

?

@contramundum53
Copy link
Member

@c-bata Could you review this PR?

@keisuke-umezawa keisuke-umezawa self-assigned this Oct 27, 2023
Copy link
Member

@keisuke-umezawa keisuke-umezawa left a comment

Choose a reason for hiding this comment

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

Code looks good to me. Also, I checked it run well on my local. Thank you for your contribution!

@keisuke-umezawa keisuke-umezawa merged commit be3106c into optuna:main Nov 1, 2023
11 checks passed
@adjeiv adjeiv deleted the trial_ordering branch November 18, 2023 10:58
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.

Sorting trials by value should only include complete trials
3 participants