Add Rec Seq2seq components and configure file, change all config files due to interface change #253
Conversation
…s due to interface change
zhtmike
changed the title
configs/rec/crnn/crnn_resnet34.yaml
Outdated
| @@ -185,7 +186,7 @@ predict: | |||
| - ToCHWImage: | |||
| # the order of the dataloader list, matching the network input and the input labels for the loss function, and optional data for debug/visaulize | |||
| output_columns: [ 'img_path', 'image', 'raw_img_shape' ] | |||
| num_columns_to_net: 1 # num inputs for network forward func | |||
| columns_indices_for_net: [0] # input indices for network forward func in output_columns | |||
There was a problem hiding this comment.
经确认pred pipeline目前是不会用到num_columns_to_net的，所以这个参数配置已删除。
| - ToCHWImage: | ||
| # the order of the dataloader list, matching the network input and the input labels for the loss function, and optional data for debug/visaulize | ||
| output_columns: ["image", "text_seq"] #, 'length'] #'img_path'] | ||
| columns_indices_for_net: [0, 1] # input indices for network forward func in output_columns |
There was a problem hiding this comment.
这里不用指定column_indices_for_label?
There was a problem hiding this comment.
column_indices_for_label如果不提供会 implicitly 用indices = [1]做为label。现已添加。
| @@ -280,3 +377,13 @@ def __call__(self, data): | |||
| assert trans.num_classes==96 | |||
| assert out['length'] ==len(text), 'Not equal: {}, {}'.format(out['length'], text) # use_space_char=False, but the dict contains space, % is also in dict | |||
| print(seq) | |||
There was a problem hiding this comment.
这些中间的print是否必要? 如仅调试用，应删去。
| @@ -33,13 +33,16 @@ def __init__(self, config: dict): | |||
|
|
|||
| self.model_name = f'{backbone_name}_{neck_name}_{head_name}' | |||
|
|
|||
| def construct(self, x): | |||
| def construct(self, x, y=None): | |||
There was a problem hiding this comment.
需要补充说明入参，x和y。并更新mindocr/models/README.md相应的接口描述。
There was a problem hiding this comment.
已补充docstring和更新接口描述。
There was a problem hiding this comment.
I wonder how we will deal with the problem if the backbone or neck will require an extra input.
There was a problem hiding this comment.
Current we don't support this feature.
README.md
Outdated
There was a problem hiding this comment.
建议优化命名：
columns_indices_for_net -> net_input_column_index
columns_indices_for_label -> label_column_index
columns_indices_for_meta_data -> meta_data_column_index
另外，meta_data_column_index 应避免手动指定，output_columns中除去net_input_column_index 和 label_column_index部分剩下都是meta data.
There was a problem hiding this comment.
已更新命名。meta_data_column_index已按这逻辑更新。
| if self.label_indices is not None: | ||
| gt = [data[x] for x in self.label_indices] | ||
| else: | ||
| gt = [data[1]] |
There was a problem hiding this comment.
| gt = [data[1]] | |
| start_idx = 0 if self.input_indices is None else len(self.input_indices) | |
| gt = [data[start_idx:]] |
I suggest using the rest of the axes not used in self.input_indices as labels when self.label_indices is None.
There was a problem hiding this comment.
The rest of the axes will be marked as meta_data.
There was a problem hiding this comment.
Can it be gt = [data[start_idx:end_idx]] where end_idx is calculated based on self.meta_data_indices the same way as start_idx does?
There was a problem hiding this comment.
We use indicies here explicitly instead of range. Since the column property can be overlapped, for example, label_1 can be used as network input, and label, as well as marked as meta data in the same time.
There was a problem hiding this comment.
We can assume that the indices are continuous for the default case, so we can avoid putting default values into config files.
| num_columns_to_net: 1 # num inputs for network forward func | ||
| num_columns_of_labels: 2 # num labels | ||
| net_input_column_index: [0] # input indices for network forward func in output_columns | ||
| label_column_index: [1, 2] # input indices marked as label |
There was a problem hiding this comment.
I feel like our config files are rapidly increasing, it's easy to make mistakes when config files are long. These fields have default values and can be safely disregarded from the config. Please see my suggestion in mindocr/utils/callbacks.py.
There was a problem hiding this comment.
yes, they have default value. net_input_column_index is default to be [0], label_column_index is default to [1]. We can create a PR later to clean the all unecessary settings.
| @@ -33,13 +33,16 @@ def __init__(self, config: dict): | |||
|
|
|||
| self.model_name = f'{backbone_name}_{neck_name}_{head_name}' | |||
|
|
|||
| def construct(self, x): | |||
| def construct(self, x, y=None): | |||
There was a problem hiding this comment.
I wonder how we will deal with the problem if the backbone or neck will require an extra input.
Thank you for your contribution to the MindOCR repo.
Before submitting this PR, please make sure:
Motivation
(Write your motivation for proposed changes here.)
Test Plan
(How should this PR be tested? Do you require special setup to run the test or repro the fixed bug?)
Related Issues and PRs
(Is this PR part of a group of changes? Link the other relevant PRs and Issues here. Use https://help.github.com/en/articles/closing-issues-using-keywords for help on GitHub syntax)