Skip to content

Conversation

@xinghai-sun
Copy link
Contributor

Resolved #226

@xinghai-sun xinghai-sun requested review from kuke and pkuyym September 4, 2017 16:29
@xinghai-sun
Copy link
Contributor Author

xinghai-sun commented Sep 4, 2017

Should we allow more than 80 columns for this part of codes, making it look better?

It will look like:
screen shot 2017-09-05 at 12 44 47 am

Another choice is that we just forget about the column alignment and follows the normal python coding style.

add_arg('host_port', int, 8086, "Server's IP port.")
add_arg('host_ip', str,
'localhost',
"Server's IP address.")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should we allow more than 80 columns of codes in this part to make it look better?

Copy link
Collaborator

@kuke kuke left a comment

Choose a reason for hiding this comment

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

Almost LGTM

from data_utils.utils import read_manifest


def add_arg(argname, type, default, help, **kwargs):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we move down or out this function?

add_arg('batch_size', int, 128, "Minibatch size.")
add_arg('trainer_count', int, 8, "# of Trainers (CPUs or GPUs).")
add_arg('beam_size', int, 500, "Beam search width.")
add_arg('parallels_bsearch',int, 12, "# of CPUs for beam search.")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe we'd better keep the prefix num_ here, can use num_proc_beam_search or something like that.

add_arg('decoder_method', str,
'ctc_beam_search',
"Decoder method. Options: ctc_beam_search, ctc_greedy",
choices = ['ctc_beam_search', 'ctc_greedy'])
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should beam_search and greedy be enough?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Add "ctc" to be more specific.

add_arg('alpha', float, 0.36, "Coef of LM for beam search.")
add_arg('beta', float, 0.25, "Coef of WC for beam search.")
add_arg('cutoff_prob', float, 0.99, "Cutoff probability for pruning.")
add_arg('use_gru', bool, False, "Use GRUs instead of Simple RNNs.")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Simple -> simple

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

add_arg('lang_model_path', str,
'lm/data/common_crawl_00.prune01111.trie.klm',
"Filepath for language model.")
add_arg('decoder_method', str,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe decode_method or decoding_method would be better.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

add_arg('batch_size', int, 128, "Minibatch size.")
add_arg('trainer_count', int, 8, "# of Trainers (CPUs or GPUs).")
add_arg('beam_size', int, 500, "Beam search width.")
add_arg('parallels_bsearch',int, 12, "# of CPUs for beam search.")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe we'd better keep the prefix num_, and can use num_proc_bsearch, num_proc_data or something like that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

Copy link
Contributor

@pkuyym pkuyym left a comment

Choose a reason for hiding this comment

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

LGTM++

@xinghai-sun xinghai-sun changed the title Reduce the config parsing codes for DS2 and make it looks cleaner. Reduce the config parsing codes (-360 Lines) for DS2 and make it looks cleaner. Sep 5, 2017
@xinghai-sun xinghai-sun merged commit b56a548 into PaddlePaddle:develop Sep 5, 2017
@xinghai-sun xinghai-sun deleted the refactor_config branch September 5, 2017 08:42
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