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

Re-design command options for testing for better understanding #411

Merged
merged 12 commits into from
Dec 5, 2016

Conversation

backyes
Copy link
Contributor

@backyes backyes commented Nov 9, 2016

Fixes #283

@backyes
Copy link
Contributor Author

backyes commented Nov 9, 2016

这个PR核心重点是尽量确保修改后版本相对原版本的test_period的默认行为一致,或者更加接近人的直觉,可能是适当牺牲一些兼容性。

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.01%) to 62.435% when pulling c6a0298 on backyes:bugfix_test_period into 05204af on baidu:develop.

"Run test every so many train batches."
" 0 for testing after each pass."
" If not 0, test log_period batches."
" If 0, test on all test data");
Copy link
Contributor

Choose a reason for hiding this comment

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

50-54行和56-59行是一样的呀。怎么区分两者的不同呢

@backyes
Copy link
Contributor Author

backyes commented Nov 9, 2016

@luotao1 此处只为占坑,并提出问题,解释核心解决的问题是啥。

@backyes
Copy link
Contributor Author

backyes commented Nov 9, 2016

  • 添加对老参数 test_period, test_all_data_in_one_period的deprecated warning提示,确保用户能对这种接口的变化观察到变化
  • 新旧接口默认表达的意义一致
  • 新接口支持表达老接口的所有功能

TODO: 相关文档的更新

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.02%) to 62.428% when pulling 0a0c55d on backyes:bugfix_test_period into 05204af on baidu:develop.

@backyes backyes changed the title 【In Progress】create PR to polish test_period meaning Re-design command options for testing for better understanding Nov 9, 2016
@backyes
Copy link
Contributor Author

backyes commented Nov 9, 2016

这个patch实现非常简单,但是主要动了用户接口,还请几位过目。

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.004%) to 62.441% when pulling 0feecbd on backyes:bugfix_test_period into 05204af on baidu:develop.

- type: int32 (default: 1000).
* `--test_period_while_training`
- Run test every so many train batches. If not 0, test log_period batches. If 0, test nothing.
- type: int32 (default: 0).
Copy link
Contributor

Choose a reason for hiding this comment

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

这个参数有必要留着么,可以用log_period参数来控制么

Copy link
Contributor Author

Choose a reason for hiding this comment

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

首先这里我理解应该是改成test_batches_while_training, 而不是log_period, 因为测试用多少batches的测试数据,跟控制训练打印日志的log_period 本质上没有关系。
已经更正为test_batches_while_training。

* `--test_all_data_in_one_period`
- This argument is usually used in testing period during traning. If true, all data will be tested in one test period. Otherwise (batch_size * log_peroid) data will be tested.
* `--test_batches_while_training`
- Test test_batches_while_training batches if test_batches_while_training != 0. If 0, test on all test data.
Copy link
Contributor

Choose a reason for hiding this comment

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

这个参数的名字感觉还是原来的好,现在没表明是和data相关

Copy link
Contributor Author

Choose a reason for hiding this comment

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

这个说来话长,我是经过仔细推敲才确定这种命名。
原来实现中, test_period既起到了决定多长时间再做test,同时特殊情况下也起到了每次test测试多少个的含义,混淆了含义,不够直观。所以,新接口目的就是使得『多长时间做test』和『每次test多少』从option上就区分出来。
所以,由于区分出每次测试多少个batch之后,例如test_batches_while_training=0的特殊值,就可以直接表达测试全部数据,非零的时候,可以表达测试多少个,这样原来那个变量就不需要了。
理解新接口思路:

  • 区分『多长时间做test』和『每次test多少』
  • 区分『训练时的test』和 『pass结束时的test』,因为两者其实根本上没有什么关系,原设计中将test_period贯穿到两者之间了。
    同时,新接口确保了和老接口默认值的等价性,并外加了warning对修改接口进行提示。
    总之,这个单纯名字上没有和data相关,我觉得可以不用考虑,data我理解也表达不出什么意义,test本身也就是对data进行test。

- type: bool (default: 1000).

* `--test_batches_while_end`
- Test test_batches_while_end batches if test_batches_while_end != 0. If 0, test on all test data.
- type: bool (default: 0).
Copy link
Contributor

Choose a reason for hiding this comment

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

这个参数是指什么?

Copy link
Contributor Author

@backyes backyes Nov 9, 2016

Choose a reason for hiding this comment

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

done。 重新增强了描述。表示的是,在pass结束后,做多少个batches的test

@backyes
Copy link
Contributor Author

backyes commented Nov 9, 2016

@luotao1 comments 注意聚合, 否则邮件太多了,:-)

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.02%) to 62.428% when pulling c509908 on backyes:bugfix_test_period into 05204af on baidu:develop.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.02%) to 62.428% when pulling b62c80f on backyes:bugfix_test_period into 05204af on baidu:develop.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.02%) to 62.423% when pulling b62c80f on backyes:bugfix_test_period into 05204af on baidu:develop.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.004%) to 62.441% when pulling b62c80f on backyes:bugfix_test_period into 05204af on baidu:develop.

@backyes backyes added this to the 0.9.0 milestone Nov 10, 2016
" If 0, test on all test data.");
P_DEFINE_bool(test_all_data_in_one_period, false,
"This option was deprecated, use test_batches_while_training "
"and test_batches_while_end instead");
Copy link
Contributor

@qingqing01 qingqing01 Nov 10, 2016

Choose a reason for hiding this comment

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

我觉得这些参数太多了、太绕了。我的意见:

  1. test_period保留,如果设置,表示测试间隔数;如果不设置,默认每个pass测试一次。 用户不想测试,test.list的位置是None就可以了。
  2. 每次都测试全部数据,相当于test_all_data_in_one_period默认是true。 如果用户想测试少量数据,自己减少test.list中的文件就可以了。因为测试间隔通常较大,不会占用非常多时间。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@qingqing01 赞同你的观点。从用户理解角度,@qingqing01 的思路更加简单,新接口试图保持和原接口语意等同等兼容性,仍然将『测试多少数据』这个属性保留了,但是相对@qingqing01的思路,还是不够简洁。

@luotao1 @reyoung @emailweixu @wangkuiyi 非常需要你们大家的意见,希望能快速解决这个问题。

Copy link
Contributor

Choose a reason for hiding this comment

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

我同意@qingqing01 的观点,现在参数太多了,而且仅从参数名字上看,不能很好的理解参数的含义。

Copy link
Collaborator

Choose a reason for hiding this comment

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

不赞同

抽样测试是非常有用的功能点,paddle必须保留可以在总的测试集合中,每次测试一部分数据的能力。这个功能对于解决很多实际问题,非常有用。可以减少测试的时间,对整体训练进程的速度提升比较大。

我们需要将test all data in one period 这个bool值,改成一个整数值。比如 test batches,默认情况下是0,即为测试所有数据,否则便是测试多少batch的数据。这样两个参数控制这个问题,test period和test batches。参数名字也比较容易理解。luotao和qingqing说的参数多的问题也解决了,对吧。

另一个问题,这么修改之后,也需要保证所有文档和demo中的使用正确性。虽然不一定在这个pr搞定,但是至少要记住或者加个issue

Copy link
Collaborator

Choose a reason for hiding this comment

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

特别是对于企业实际问题,很多情况下都是欠拟合,测试集合可能特别大。并没有必要在训练的所有阶段,关注全量测试的结果。

所以,这个功能还是必须保留的。

Copy link
Contributor Author

@backyes backyes Nov 16, 2016

Choose a reason for hiding this comment

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

如果用户想测试少量数据,自己减少test.list中的文件就可以了。

@qingqing01 提议的思路仍然保留了对少部分数据进行test的。

我理解从需求上看,分成几个问题:

  1. test_period保留,如果设置,表示测试间隔数;如果不设置,默认每个pass测试一次。 用户不想测试,test.list的位置是None就可以了。
  • 是否要支持,同时在训练过程中进行test和pass结束后进行test。@qingqing 的建议下,是不支持这个功能的。

每次都测试全部数据,相当于test_all_data_in_one_period默认是true。 如果用户想测试少量数据,自己减少test.list中的文件就可以了。

  • 是否要支持,同时在训练的时候test部分数据,但是pass结束后的test跑全量的test数据。

如果上述两个需求必须同时满足,那么各位的建议的改法,好像都不能满足需求。大家再看看哈。

Copy link
Collaborator

Choose a reason for hiding this comment

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

自己减少test.list和测试的时候每次抽样一小部分作为测试集合,是完全不同的两个结果。

比如全部集合是0到10的集合,减少test.list是我全部训练过程中,只测试0。而每次抽样一部分是说,这次是0,下次可能是1,下下次可能是2。

效果完全不一样

Copy link
Collaborator

Choose a reason for hiding this comment

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

我不太清楚我建议的改发为什么不能满足需求?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

抱歉才回复。

我们需要将test all data in one period 这个bool值,改成一个整数值。比如 test batches,默认情况下是0,即为测试所有数据,否则便是测试多少batch的数据。这样两个参数控制这个问题,test period和test batches。参数名字也比较容易理解。luotao和qingqing说的参数多的问题也解决了,对吧。

我理解,仅test period和test batches 这两个参数不能描述区分,这样一种情况

训练阶段每隔1000个batches 测试一次1000个sample的test数据;每个pass结束后,测试全量test数据集。

因为无法区分训练阶段的test样本数目和pass结束后的test样本数。 你们觉得呢?

@reyoung
Copy link
Collaborator

reyoung commented Nov 16, 2016

自己减少test.list和测试的时候每次抽样一小部分作为测试集合,是完全不同的两个结果。

比如全部集合是0到10的集合,减少test.list是我全部训练过程中,只测试0。而每次抽样一部分是说,这次是0,下次可能是1,下下次可能是2。

效果完全不一样

发自 Smartisan T2

backyes [email protected] 于 2016年11月16日,上午9:52写道:

@backyes commented on this pull request.


In paddle/trainer/Trainer.cpphttps://github.com//pull/411:

            " If 0, test on all test data");

+P_DEFINE_int32(test_batches_while_end, 0,

  •           "test test_batches_while_end batches at pass end."
    
  •           " Always run test at pass end."
    
  •           " If not 0, test test_batches_while_end batches."
    
  •           " If 0, test on all test data.");
    
    +P_DEFINE_bool(test_all_data_in_one_period, false,
  •           "This option was deprecated, use test_batches_while_training "
    
  •           "and test_batches_while_end instead");
    

如果用户想测试少量数据,自己减少test.list中的文件就可以了。
@qingqing01https://github.com/qingqing01 提议的思路仍然保留了对少部分数据进行test的。

我理解从需求上看,分成几个问题:

  1. test_period保留,如果设置,表示测试间隔数;如果不设置,默认每个pass测试一次。 用户不想测试,test.list的位置是None就可以了。
  2. 是否要支持,同时在训练过程中进行test和pass结束后进行test。@qingqinghttps://github.com/qingqing 的建议下,是不支持这个功能的。每次都测试全部数据,相当于test_all_data_in_one_period默认是true。 如果用户想测试少量数据,自己减少test.list中的文件就可以了。
  3. 是否要支持,同时在训练的时候test部分数据,但是pass结束后的test跑全量的test数据。

如果上述两个需求必须同时满足,那么各位的建议的改法,好像都不能满足需求。大家再看看哈。


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHubhttps://github.com//pull/411, or mute the threadhttps://github.com/notifications/unsubscribe-auth/AAsee3bS_G2pD48Dp9oTJo8bzIwuT3E_ks5q-mG8gaJpZM4KtUzv.

@backyes
Copy link
Contributor Author

backyes commented Nov 17, 2016

自己减少test.list和测试的时候每次抽样一小部分作为测试集合,是完全不同的两个结果。

比如全部集合是0到10的集合,减少test.list是我全部训练过程中,只测试0。而每次抽样一部分是说,这次是0,下次可能是1,下下次可能是2。

效果完全不一样

@reyoung 这个说的很有道理,所以我们还是必须保存全量的test集,即便只测试总测试集中的部分样本。

@backyes
Copy link
Contributor Author

backyes commented Nov 28, 2016

@reyoung @qingqing01 @wangkuiyi
经过上次会议讨论,明确结果是只保留test_period参数:

  • test_period=N (N !=0), 每N个batches过后,进行test,且test全量数据
  • test_period=0, 训练阶段不进行test,仅仅在每个pass过后进行test,且test全量数据

对用户可能影响:

  • 修正后的参数意义和原参数意义将不一致,不再存在对部分数据进行测试的场景,潜在影响可能会导致部分用户按照老规则指定了过多test数据,导致整体训练速度变慢。

下面将提交相关patch。

@reyoung
Copy link
Collaborator

reyoung commented Nov 28, 2016

是的,是这么搞。

@backyes
Copy link
Contributor Author

backyes commented Nov 28, 2016

@qingqing01

由于模型average 的存在,test将在average的model上做test,所以请问,training中的test使用的模型是否跟pass结束以后的测试模型都是采用average模型?

这个和本issue问题实现有重大关系。(我也review下代码)

 * always do test on all test data
 * do test at the end of each pass if test_period=0, otherwise do test if test_period batches passed
@backyes
Copy link
Contributor Author

backyes commented Nov 28, 2016

@reyoung @qingqing01 @luotao1

由于这个patch较最早版本变化很大,如果觉得review很不方便,我可以重新开一个pr

@backyes
Copy link
Contributor Author

backyes commented Nov 28, 2016

@qingqing01

由于模型average 的存在,test将在average的model上做test,所以请问,training中的test使用的模型是否跟pass结束以后的测试模型都是采用average模型?

这个和本issue问题实现有重大关系。(我也review下代码)

经过分析,average模型相关的内容主要适合evaluator, 跟test核心逻辑无关,所以不用这里对test_period的修改无需考虑average模型相关部分。 @qingqing01 如果有疑问可以在这个pr中给出建议。

- Run testing every test_period train batches. If not set, run testing each pass.
- type: int32 (default: 1000).
- if equal 0, do test on all test data at the end of each pass while if equal non-zero, do test on all test data once each test_period batches passed while training is going on.
- type: int32 (default: 0).
Copy link
Contributor

Choose a reason for hiding this comment

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

if equal 0, do test on all test data at the end of each pass. While if equal non-zero, do test on all test data every test_period batches.

下面好几处,一起改下吧~

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

@qingqing01 qingqing01 merged commit 239cade into PaddlePaddle:develop Dec 5, 2016
Copy link
Collaborator

@reyoung reyoung left a comment

Choose a reason for hiding this comment

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

由于这块逻辑简化了,所以其实batches参数也并没有什么用了。可以直接简化成while循环。。

@backyes

} else {
num = testDataProvider_->getNextBatch(batchSize, &dataBatch);
}
break;
Copy link
Collaborator

Choose a reason for hiding this comment

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

startTestPeriod();
while(int num = testDataProvider_->getNextBatch(batchSize, &dataBatch)) {
   testOneDataBatch(dataBatch, &outArgs);
}
testDataProvider_->reset();
if (intconfig_->prevBatchState) {
  gradientMachine_->resetState();
}
finishTestPeriod();

zhhsplendid pushed a commit to zhhsplendid/Paddle that referenced this pull request Sep 25, 2019
thisjiang pushed a commit to thisjiang/Paddle that referenced this pull request Oct 28, 2021
wangxicoding pushed a commit to wangxicoding/Paddle that referenced this pull request Dec 9, 2021
AnnaTrainingG pushed a commit to AnnaTrainingG/Paddle that referenced this pull request Sep 19, 2022
lizexu123 pushed a commit to lizexu123/Paddle that referenced this pull request Feb 23, 2024
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.

7 participants