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

Reformat docstrings in espnet/asr #914

Merged
merged 15 commits into from
Jul 2, 2019
Merged

Conversation

Masao-Someki
Copy link
Contributor

@Masao-Someki Masao-Someki commented Jun 22, 2019

  • Reformat docstrings of the following files.
  • ./espnet/asr/asr_mix_utils.py
  • ./espnet/asr/asr_utils.py
  • ./espnet/asr/chainer_backend/asr.py
  • ./espnet/asr/pytorch_backend/asr.py
  • ./espnet/asr/pytorch_backend/asr_mix.py
  • Fixed typo

* reformat docstrings of the following files.
[x] ./espnet/asr/asr_mix_utils.py
[x] ./espnet/asr/asr_utils.py
[x] ./espnet/asr/chainer_backend/asr.py
[ ] ./espnet/asr/pytorch_backend/asr.py
[ ] ./espnet/asr/pytorch_backend/asr_mix.py

fixed typo
@Masao-Someki Masao-Someki changed the base branch from master to v.0.5.0 June 22, 2019 09:30
@kan-bayashi
Copy link
Member

kan-bayashi commented Jun 22, 2019

Thank you so much!

I have some requests and comments.

  • Capitalize the first word in each sentence
  • Add period the end of sentence
  • No need to add docstring to private method
  • No need to use variable name in Returns:

e.g.

# now
Returns:
    outputs (Tensor): batch of output tensors (B, T, D)

# after
Returns:
    Tensor: Batch of output tensors (B, T, D).

@Masao-Someki
Copy link
Contributor Author

Okay!

@sw005320 sw005320 added this to the v.0.5.0 milestone Jun 22, 2019
@Masao-Someki
Copy link
Contributor Author

I couldn't figure out the type of score at line 482 in ./asr_utils.py
Could you tell me where I can find out the type?

* fixed pytorch_backend
* fixed some typos in `./asr_mix_utils.py` , `asr_utils.py` ,
  `./chainer_backend/asr.py` .

* fixed typos
@kan-bayashi
Copy link
Member

score = float(hyp['score'])

Maybe float.

@Masao-Someki
Copy link
Contributor Author

Masao-Someki commented Jun 22, 2019

Oh...I don't know why I miss that sentence.
Thank you!

@Masao-Someki
Copy link
Contributor Author

I'll add details to data-type in args and results tomorrow.

ex) Dict → Dict[String, Dict[String. List[String]]]

@codecov
Copy link

codecov bot commented Jun 22, 2019

Codecov Report

Merging #914 into v.0.5.0 will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff            @@
##           v.0.5.0     #914   +/-   ##
========================================
  Coverage    49.32%   49.32%           
========================================
  Files          100      100           
  Lines        10724    10724           
========================================
  Hits          5290     5290           
  Misses        5434     5434
Impacted Files Coverage Δ
espnet/asr/asr_mix_utils.py 0% <ø> (ø) ⬆️
espnet/asr/pytorch_backend/asr_mix.py 0% <ø> (ø) ⬆️
espnet/asr/chainer_backend/asr.py 0% <ø> (ø) ⬆️
espnet/asr/asr_utils.py 26.1% <ø> (ø) ⬆️
espnet/asr/pytorch_backend/asr.py 0% <ø> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5a966c3...d345e43. Read the comment docs.

@kan-bayashi
Copy link
Member

It is better to unify the type format e.g. str or String, Dict or dict.
I like lower-case e.g. str, dict, list

* update data_type
@Masao-Someki
Copy link
Contributor Author

I coudn't find out the following, so please collect or teach me where to watch...

  • espnet/asr/chainer_backend/asr.py: L100: type of device
  • espnet/asr/asr_mix_utils.py:L103: type of device
  • espnet/asr/asr_utils.py:L426:type of trainer
  • espnet/asr/chainer_backend/asr.py: L103, L162: explanation of accum_grad.

@kan-bayashi
Copy link
Member

kan-bayashi commented Jun 24, 2019

espnet/asr/chainer_backend/asr.py: L100: type of device

device (int or dict): The destination device info to send variables. In the case of cpu or single gpu, `device=-1 or 0`, respectively.
    In the case of multi-gpu, `device={"main":0, "sub_1": 1, ...}`.

espnet/asr/asr_mix_utils.py:L103: type of device

device (torch.device): The destination device to send tensor.

espnet/asr/asr_utils.py:L426:type of trainer

trainer (chainer.training.Trainer): Chainer's trainer instance.

espnet/asr/chainer_backend/asr.py: L103, L162: explanation of accum_grad.

accum_grad (int): The number of gradient accumulation. if set to 2, the network parameters will be updated once in twice, i.e. actual batchsize will be doubled.

Could you remove following highlighted warning?
https://travis-ci.org/espnet/espnet/jobs/549262746#L1954-L1956

@Masao-Someki
Copy link
Contributor Author

I fixed the highlighted warnings

fixed typo
@kan-bayashi
Copy link
Member

If ready to merge, please let me know.
I will review.

@Masao-Someki
Copy link
Contributor Author

It's ready. Could you review it?

@kan-bayashi
Copy link
Member

I will check today or tomorrow.

"""Make batch set from json dictionary.

Args:
data (dict[str, list[Any]): Dictionary loaded from data.json.
Copy link
Member

Choose a reason for hiding this comment

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

typo: dict[str, list[Any]]

batch_size (int): Batch size.
max_length_in (int): Maximum length of input to decide adaptive batch size.
max_length_out (int): Maximum length of output to decide adaptive batch size.
num_batches (int): # Number of batches to use (for debug).
Copy link
Member

Choose a reason for hiding this comment

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

typo #?

min_batch_size (int): Mininum batch size (for multi-gpu).

Returns:
list[tuple[str, dict[str, list[dict[str, Any]]]]: List of batches.
Copy link
Member

Choose a reason for hiding this comment

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

typo: list[tuple[str, dict[str, list[dict[str, Any]]]]] list (1) of tuple (2) of dict (3) of list (4) of dict(5), you need 5 ]. LOL

@ShigekiKarita
Copy link
Member

@kan-bayashi

It is better to unify the type format e.g. str or String, Dict or dict.
I like lower-case e.g. str, dict, list

I have different opinion. Because there are PEPs for type annotations, we can use them as clear rules. For example, they should be str, Dict, List

https://www.python.org/dev/peps/pep-0484/
https://www.python.org/dev/peps/pep-0526/

:param eta float {0.01,0.3,1.0}
"""
Args:
model (Torch model): model.
Copy link
Member

Choose a reason for hiding this comment

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

model (torch.nn.Module): model.?

@@ -169,8 +196,12 @@ def _restore_snapshot(model, snapshot, load_fn=chainer.serializers.load_npz):


def adadelta_eps_decay(eps_decay):
"""Extension to perform adadelta eps decay"""
"""Extension to perform adadelta eps decay.
Copy link
Member

Choose a reason for hiding this comment

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

Args:
    eps_decay (float): decay rate of eps

@kan-bayashi
Copy link
Member

Thank you for your comments, Shigeki.
OK. We should follow these rules.

Fixed typos

fixed typo

this is a commit for typos.
I tried a few times, so I rebase some commits.
Copy link
Member

@kan-bayashi kan-bayashi left a comment

Choose a reason for hiding this comment

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

Thank you so much.
Could you resolve conflict?

"""Visualize attention weights matrix.

Args:
att_w(Tensor): attention weight matrix
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
att_w(Tensor): attention weight matrix
att_w(Tensor): Attention weight matrix.


Args:
js (dict[str, Any]): Groundtruth utterance dict.
nbest_hyps_sd (list[dict[str, Any]]): List of hypothesis for multi_speakers: nutts x nspkrs.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
nbest_hyps_sd (list[dict[str, Any]]): List of hypothesis for multi_speakers: nutts x nspkrs.
nbest_hyps_sd (list[dict[str, Any]]): List of hypothesis for multi_speakers (# Utts x # Spkrs).


Args:
eps_decay (float): decay rate of eps
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
eps_decay (float): decay rate of eps
eps_decay (float): Decay rate of eps.

:param eta float {0.01,0.3,1.0}
"""
Args:
model (torch.nn.model): model.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
model (torch.nn.model): model.
model (torch.nn.model): Model.

"""
Args:
model (torch.nn.model): model.
iteration (int): number of iteration.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
iteration (int): number of iteration.
epoch (int): Number of epochs.

device (torch.device): The device to send to.

Returns:
tuple(torch.Tensor, torch.Tensor, torch.Tensor)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
tuple(torch.Tensor, torch.Tensor, torch.Tensor)
tuple(torch.Tensor, torch.Tensor, torch.Tensor): Transformed batch.

@@ -12,12 +12,12 @@
import matplotlib
import numpy as np

'''
"""
reuse modules in asr_utils:
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
reuse modules in asr_utils:
Utility functions for ASR mix recipes. Reuse following modules in asr_utils:

@@ -12,12 +12,12 @@
import matplotlib
import numpy as np

'''
"""
reuse modules in asr_utils:
CompareValueTrigger, restore_snapshot, _restore_snapshot, adadelta_eps_decay,
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
CompareValueTrigger, restore_snapshot, _restore_snapshot, adadelta_eps_decay,
CompareValueTrigger, restore_snapshot, adadelta_eps_decay,

@@ -12,12 +12,12 @@
import matplotlib
import numpy as np

'''
"""
reuse modules in asr_utils:
CompareValueTrigger, restore_snapshot, _restore_snapshot, adadelta_eps_decay,
_adadelta_eps_decay, torch_snapshot, _torch_snapshot_object, AttributeDict,
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
_adadelta_eps_decay, torch_snapshot, _torch_snapshot_object, AttributeDict,
chainer_load, torch_snapshot, torch_save, torch_load, torch_resume

@@ -12,12 +12,12 @@
import matplotlib
import numpy as np

'''
"""
reuse modules in asr_utils:
CompareValueTrigger, restore_snapshot, _restore_snapshot, adadelta_eps_decay,
_adadelta_eps_decay, torch_snapshot, _torch_snapshot_object, AttributeDict,
get_model_conf, chainer_load, torch_save, torch_load, torch_resume
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
get_model_conf, chainer_load, torch_save, torch_load, torch_resume
AttributeDict, get_model_conf

* update docstring for review.
@Masao-Someki
Copy link
Contributor Author

Masao-Someki commented Jun 27, 2019

I wonder why L86 in asr_utils.py in branch v.0.5.0 says MT iaxi="1".

:param int iaxis: dimension to access input (for ASR iaxis=0, for MT iaxis="1".)

The parametor is set as integer value, so I think it should be MT iaxis=1. Should I set "1" as a string in docstring?

* update kan-bayashi's review
* resolve conflicts
@kan-bayashi
Copy link
Member

dec_len = int(self.data[idx][1][self.ikey][self.iaxis]['shape'][0])

It seems iaxis should be int.

@Masao-Someki
Copy link
Contributor Author

I think all the docstrings are updated, with no typos..
I don't know why codedec has an error. Do you have any idea to solve it?

@kan-bayashi
Copy link
Member

Sorry for late reply.
Could you solve conflict?
Then we will merge.

@Masao-Someki
Copy link
Contributor Author

Masao-Someki commented Jul 2, 2019

I fixed the conflicts.
I clicked "commit merge" button by mistake. Sorry about that if it confuses you..

@kan-bayashi kan-bayashi changed the title [WIP] reformat docstrings in ./espnet/asr Reformat docstrings in ./espnet/asr Jul 2, 2019
@kan-bayashi kan-bayashi changed the title Reformat docstrings in ./espnet/asr Reformat docstrings in espnet/asr Jul 2, 2019
Masao-Someki and others added 2 commits July 2, 2019 18:29
Co-Authored-By: Tomoki Hayashi <hayashi.tomoki@g.sp.m.is.nagoya-u.ac.jp>
Co-Authored-By: Tomoki Hayashi <hayashi.tomoki@g.sp.m.is.nagoya-u.ac.jp>
@sw005320
Copy link
Contributor

sw005320 commented Jul 2, 2019

thanks a lot!

@sw005320 sw005320 merged commit 76d0492 into espnet:v.0.5.0 Jul 2, 2019
@Masao-Someki Masao-Someki deleted the docstring branch July 19, 2019 03:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants