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

Fix small bugs in hpopt #873

Merged
merged 10 commits into from
May 28, 2024
Merged

Conversation

akshatzalte
Copy link
Contributor

Description

Hyperparameter optimization was giving very small (~10^-10) Learning rates, which seemed wrong to people in the devs meeting (5/20/2024). This was because the search space was too vast.

Also, the batch-size was not enforced to be an integer currently.

Reduction of search space and fixing the values in hpopt.

Example / Current workflow

In the current workflow, quniform outputs a float value which is not correct.
For the batch-size, tune.choice() used to simplify it and enforce only 16, 32.., 256 is selected.

Bugfix / Desired workflow

Reduced the search space for the init and final lr ratios and max_lr Fixed the warmup epochs to be an integer (and not None) Fixed the batch size to be an integer (powers of 2)
Replace quniform to qrandint to ensure int for agrregation_norm, depth, ffn_hidden_dim, ffn_num_layers, message_hidden_dim.

Questions

none

Relevant issues

#851

Checklist

Reduced the search space for the init and final lr ratios and max_lr
Fixed the warmup epochs to be an integer (and not None)
Fixed the batch size to be integer (powers of 2)
Replace quniform to qrandint to ensure int for agrregation_norm, depth, ffn_hidden_dim, ffn_num_layers, message_hidden_dim.
@akshatzalte akshatzalte added this to the v2.0.1 milestone May 21, 2024
@akshatzalte akshatzalte requested a review from KnathanM May 21, 2024 17:13
@akshatzalte akshatzalte linked an issue May 21, 2024 that may be closed by this pull request
@shihchengli shihchengli self-requested a review May 21, 2024 18:34
Copy link
Contributor

@KnathanM KnathanM left a comment

Choose a reason for hiding this comment

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

Thanks for the PR!

chemprop/cli/hpopt.py Outdated Show resolved Hide resolved
chemprop/cli/hpopt.py Outdated Show resolved Hide resolved
chemprop/cli/hpopt.py Outdated Show resolved Hide resolved
Copy link
Member

@JacksonBurns JacksonBurns left a comment

Choose a reason for hiding this comment

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

Couple small questions/suggestions. Thanks for starting this!

chemprop/cli/hpopt.py Outdated Show resolved Hide resolved
chemprop/cli/hpopt.py Outdated Show resolved Hide resolved
Copy link
Contributor

@shihchengli shihchengli left a comment

Choose a reason for hiding this comment

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

Thanks for making this PR.

chemprop/cli/hpopt.py Show resolved Hide resolved
@akshatzalte
Copy link
Contributor Author

I have addressed all the comments. Please re-review and merge!

@akshatzalte
Copy link
Contributor Author

Made some more fixes re: warmup epochs. Open for re-review.

chemprop/cli/hpopt.py Outdated Show resolved Hide resolved
chemprop/cli/hpopt.py Outdated Show resolved Hide resolved
Copy link
Contributor

@shihchengli shihchengli left a comment

Choose a reason for hiding this comment

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

LGTM

@akshatzalte
Copy link
Contributor Author

All comments addressed. Open for re-review and merge

@JacksonBurns
Copy link
Member

@akshatzalte one last small thing, then it's good to go.

@JacksonBurns JacksonBurns enabled auto-merge (squash) May 28, 2024 16:26
@JacksonBurns JacksonBurns self-requested a review May 28, 2024 19:41
@JacksonBurns JacksonBurns merged commit 2af3681 into chemprop:main May 28, 2024
13 checks passed
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.

[v2 BUG]: Non integer values for 'batch-size' in hpopt output
4 participants