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

Our first PR! #1

Merged
merged 2 commits into from
Apr 15, 2021
Merged

Our first PR! #1

merged 2 commits into from
Apr 15, 2021

Conversation

Eleven1Liu
Copy link
Collaborator

@Eleven1Liu Eleven1Liu commented Apr 14, 2021

This is the first pull request.

@Eleven1Liu Eleven1Liu self-assigned this Apr 14, 2021
@Eleven1Liu Eleven1Liu requested review from a team as code owners April 14, 2021 05:21
henryyang42
henryyang42 previously approved these changes Apr 14, 2021
@log.enter('load_dataset')
def load_dataset(config):
"""Preparing TextDataset from raw data."""
data_dir = os.path.join(config['data_dir'], config['data_name'])
Copy link
Contributor

@henryyang42 henryyang42 Apr 14, 2021

Choose a reason for hiding this comment

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

Unify all config['xxx'] syntax to config.xxx?

Choose a reason for hiding this comment

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

Some hyperparameter search tools may require configs as a dictionary.
Correct me if I'm wrong.

main.py Outdated Show resolved Hide resolved
parser.add_argument('--display_iter', type=int, default=100, help='Log state after every n epochs (default: %(default)s)')
parser.add_argument('--data_workers', type=int, default=1, help='Use multi-cpu core for data pre-processing (default: %(default)s)')
parser.add_argument('--eval', action='store_true', help='Only run evaluation on the test set (default: %(default)s)')
parser.add_argument('--load_checkpoint', help='The checkpoint to warm-up with (default: %(default)s)')
Copy link
Contributor

Choose a reason for hiding this comment

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

load_checkpoint is more like an action to me. Rename to, for example checkpoint_path, to match the description The checkpoint to warm-up with?

sian-chen
sian-chen previously approved these changes Apr 14, 2021
@Eleven1Liu Eleven1Liu merged commit 44ee688 into master Apr 15, 2021
@Eleven1Liu Eleven1Liu deleted the init-commit branch April 15, 2021 03:16
Eleven1Liu added a commit that referenced this pull request Jul 3, 2021
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.

3 participants