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.

Sign up for GitHub

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

Jump to bottom

Add depthconv_conv2d tests #9225

Merged
merged 3 commits into from
Feb 2, 2018
Merged

Add depthconv_conv2d tests #9225

merged 3 commits into from
Feb 2, 2018

Conversation

taehoonlee
Copy link
Contributor

This PR adds depthconv_conv2d tests by immigrating existing codes from "applications_test.py" to "convolutional_test.py". As the "application_test.py" begins to run conditionally, regular tests can not hit depthconv_conv2d (lines 3503-3522 in keras/backend/tensorflow_backend.py, see one of the recent builds).

@taehoonlee
Copy link
Contributor Author

The results are:

keras/backend/tensorflow_backend.py        1197     98    92%   111, 126-129, 146, 167, 173-174, 200, 211, 224, 255, 521-522, 692, 802, 986, 1061, 1323, 1558, 1679, 1782-1783, 1873, 2093, 2184, 2228, 2432, 2435, 2438, 2458, 2463, 2497-2498, 2585, 2607, 2620, 2625, 2693, 2700, 2726, 2796, 2817, 2819, 2822, 2871, 2899, 2940, 3181, 3187, 3202, 3208, 3223, 3229, 3273, 3287, 3319, 3360, 3362, 3374-3375, 3381, 3411, 3413, 3421-3422, 3463, 3465, 3472, 3503-3522, 3546, 3585, 3587, 3600-3601, 3607, 3648-3649, 3697-3698, 3740, 3747, 3752, 3757, 4068, 4118, 4120

keras/backend/tensorflow_backend.py        1197     88    93%   111, 126-129, 146, 167, 173-174, 200, 211, 224, 255, 521-522, 692, 802, 986, 1061, 1323, 1558, 1679, 1782-1783, 1873, 2093, 2184, 2228, 2432, 2435, 2438, 2458, 2463, 2497-2498, 2585, 2607, 2620, 2625, 2693, 2700, 2726, 2796, 2817, 2819, 2822, 2871, 2899, 2940, 3181, 3187, 3202, 3208, 3223, 3229, 3273, 3287, 3319, 3360, 3362, 3374-3375, 3381, 3411, 3413, 3421-3422, 3463, 3465, 3472, 3504, 3506, 3513, 3546, 3585, 3587, 3600-3601, 3607, 3648-3649, 3697-3698, 3740, 3747, 3752, 3757, 4068, 4118, 4120

@fchollet
Copy link
Collaborator

regular tests can not hit depthconv_conv2d

Maybe we should just have a backend test for it? That could be useful if other backends start implementing depthwise_conv2d.

@taehoonlee
Copy link
Contributor Author

I agree @fchollet. But currently, the "backend_test.py" examines a backend function with an assertion among results from individual backends. In other words, the backend functions, implemented in only a single backend, are not included in the "backend_test.py". For example, neither separable_conv1d or separable_conv2d are currently not included. If that's the case, what if such backend functions were included in the "layers test"?

@fchollet
Copy link
Collaborator

I think we should be free to add backend-specific test functions in backend_test.py.

@taehoonlee
Copy link
Contributor Author

I agree @fchollet. I wrote a backend test to check functionality of depthconv_conv2d.

fchollet
fchollet reviewed Jan 31, 2018
View reviewed changes
tests/keras/layers/convolutional_test.py Outdated

@pytest.mark.skipif(K.backend() != 'tensorflow', reason='Requires TF backend')
@keras_test
def test_depthwise_conv_2d():
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we still need this test at all? Shouldn't coverage be achieved in the mobilenet tests?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@fchollet as you commented, there are redundant tests between backend tests and layer tests. I just wrote the both to fit the current testing format, but now a test_depthwise_conv_2d in layer tests has been removed.

fchollet
fchollet approved these changes Jan 31, 2018
View reviewed changes
Copy link
Collaborator

@fchollet fchollet left a comment

Choose a reason for hiding this comment

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

LGTM

@fchollet fchollet merged commit fce3e07 into keras-team:master Feb 2, 2018
@taehoonlee taehoonlee deleted the add_depthconv_tests branch February 2, 2018 01:50
ahundt added a commit to ahundt/keras that referenced this pull request Feb 16, 2018
@ahundt
Merge branch 'master' of github.com:fchollet/keras into pr_7508
e736fb4 
* 'master' of github.com:fchollet/keras: (57 commits)
  Minor README edit
  Speed up Travis tests (keras-team#9386)
  fix typo (keras-team#9391)
  Fix style issue in docstring
  Prepare 2.1.4 release.
  Fix activity regularizer + model composition test
  Corrected copyright years (keras-team#9375)
  Change default interpolation from nearest to bilinear. (keras-team#8849)
  a capsule cnn on cifar-10 (keras-team#9193)
  Enable us to use sklearn to do cv for functional api (keras-team#9320)
  Add support for stateful metrics. (keras-team#9253)
  The type of list keys was float (keras-team#9324)
  Fix mnist sklearn wrapper example (keras-team#9317)
  keras-team#9287 Fix most of the file-handle resource leaks. (keras-team#9309)
  Pass current learning rate to schedule() in LearningRateScheduler (keras-team#8865)
  Simplify with from six.moves import input (keras-team#9216)
  fixed RemoteMonitor: Json to handle np.float32 and np.int32 types (keras-team#9261)
  Update tweet length from 140 to 280 in docs
  Add `depthconv_conv2d` tests (keras-team#9225)
  Remove `force` option in progbar
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fchollet fchollet fchollet approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@taehoonlee @fchollet