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

Add from_params to Linear & Conv #7525

Merged
merged 15 commits into from
Oct 31, 2019
Merged

Conversation

crcrpar
Copy link
Contributor

@crcrpar crcrpar commented Jun 16, 2019

This addresses #7042 & #7041 by showing draft implementations of some connection links that are commonly used.

The aim of this PR is easing initializing links from weights that are not easy to generate chainer.Initializer\s or that are trained beforehand in some way (e.g. word embedding vectors or a weight extracted from a pretrained model).
Currently, users can pass those weights to initialW/initial_bias if they are numpy.ndarray as a variant of chainer.initializers.Constant, but I don't think this manner intuitive.

@crcrpar crcrpar changed the title Add from_params to Linear & Conv [RFC] Add from_params to Linear & Conv Jun 17, 2019
@crcrpar
Copy link
Contributor Author

crcrpar commented Jul 2, 2019

I think this depends on #7531 somewhat.

Any thoughts, @emcastillo?

@emcastillo
Copy link
Member

AFAIK #7531 just makes the initializers to allow chainerx arrays as arguments and disable the explicit fallback for random routines as chainerx now supports random (through chainerx internal fallback).

In this PR the proposed from_params could take any chainerx array as input and as long as chainerx supports the operations being done in that method there shouldn't be any problem.

@crcrpar
Copy link
Contributor Author

crcrpar commented Jul 3, 2019

Sorry for the lack of explanation.

The motivation of this PR is to ease Link initialization with some specific ndarrays, for instance, initializing L.EmbedID with some publicly available embedding weight.
So, the method added by this PR takes :ref:ndarray W & optional b as its arguments and passes them to the corresponding initializer-related arguments (initialW and initial_bias) of the constructor the class.

@emcastillo
Copy link
Member

Sorry for the delay.
Yes, if the from_params end up relying on initializers when calling the constructor, chainer x support is going to be needed.

This was already merged in #7687 so there shouldn't be issues now.

@crcrpar
Copy link
Contributor Author

crcrpar commented Jul 10, 2019

Thanks for the info.

I'll resume working.

@crcrpar crcrpar marked this pull request as ready for review August 10, 2019 01:00
@crcrpar
Copy link
Contributor Author

crcrpar commented Aug 12, 2019

Apologize for having been slow to work.
I'm done with this, so could you give me a review?

'cuda:0',
]
}))
class TestConvolutionNDFromParams(unittest.TestCase):
Copy link
Member

@emcastillo emcastillo Aug 19, 2019

Choose a reason for hiding this comment

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

Instead of adding a new Test case,
How about using the from_params in the create_link of the ConvolutionND(testing.LinkTestCase) main test

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure.

Is this applicable to the other test cases?

Copy link
Member

Choose a reason for hiding this comment

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

yes,
rebase with the master branch so the new tests appear :)

You can ignore this suggestion if you dont feel comfortable with this change

Copy link
Member

@emcastillo emcastillo left a comment

Choose a reason for hiding this comment

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

You will need to rebase because some of the convolution tests changed some days ago.
Thanks!!

@emcastillo emcastillo added cat:enhancement Implementation that does not break interfaces. st:awaiting-author State indicating that response is needed from contributors, often authors of pull request. labels Aug 19, 2019
@emcastillo emcastillo added this to the v7.0.0b3 milestone Aug 19, 2019
emcastillo
emcastillo previously approved these changes Aug 21, 2019
Copy link
Member

@emcastillo emcastillo left a comment

Choose a reason for hiding this comment

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

Please fix the syntax error, once it is done I think it will be ready for CI.

Thanks!

assert link2.W.shape == link1.W.shape
assert link2.b.shape == link2.b.shape
assert link2.stride == link1.stride
assert link2.pad = link1.pad
Copy link
Member

Choose a reason for hiding this comment

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

Syntax error here, should be ==

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, accessed.

@emcastillo
Copy link
Member

Another syntax error:
/home/travis/build/chainer/chainer/tests/chainer_tests/links_tests/connection_tests/test_convolution_2d.py:117:17: F821 undefined name 'L'
😅

@asi1024 asi1024 removed this from the v7.0.0b3 milestone Aug 22, 2019
@emcastillo
Copy link
Member

Fix travis errors please.
I dont know if you can run the tests locally using pytest, but it would help a lot 😃

@crcrpar
Copy link
Contributor Author

crcrpar commented Aug 22, 2019

I'm sorry, but did the ci run as we expect the last time?

@emcastillo
Copy link
Member

there are several errors on travis


link2 = convolution_nd.ConvolutionND.from_params(
link1.W, link1.b,
stride=self.stride, pad=self.pad, groups=self.group)
Copy link
Member

Choose a reason for hiding this comment

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

groups=self.groups

@crcrpar
Copy link
Contributor Author

crcrpar commented Aug 23, 2019

there are several errors on travis

I meant to say that some configuration of log display seemed to be changed since just there was only kind of "test failed" message first.


link = cls(
in_channels, out_channels, (kw, kh), stride, pad, nobias,
initialW=variable.as_array(W), initial_bias=variable.as_array(b),
Copy link
Member

@emcastillo emcastillo Aug 23, 2019

Choose a reason for hiding this comment

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

this code fails when arrays other than numpy are passed as initializers.

Running your branch in my environment shows

 Base test method: TestConvolution2D_use_chainerx_false__chainerx_device_None__use_cuda_true__cuda_device_0__use_cudnn_always__cudnn_deterministic_false__autotune_false__cudnn_fast_batch_normalization_false__use_ideep_never.test_from_params
E           Test parameters:
E             W_dtype: <class 'numpy.float64'>
E             x_dtype: <class 'numpy.float64'>
E
E
E           (caused by)
E           TypeError: invalid type of initializer: <class 'cupy.core.core.ndarray'>

We can allow to use cupy/chainerx as a constant initializer (shouldnt be difficult)
but let us discuss it first!

Copy link
Member

Choose a reason for hiding this comment

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

I just submitted a PR to tackle this issue #8022

@@ -187,4 +187,31 @@ def test_invalid_size(self):
self.link(chainer.Variable(self.x))


@testing.parameterize(*testing.product({
'nobias': [True, False],
'device': [
Copy link
Member

Choose a reason for hiding this comment

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

You can remove the devices, they are not being used in the test

'@cupy:0',
'native:0',
'cuda:0',
]
Copy link
Member

@emcastillo emcastillo Aug 23, 2019

Choose a reason for hiding this comment

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

ditto

@emcastillo
Copy link
Member

Check CI errors

@crcrpar
Copy link
Contributor Author

crcrpar commented Oct 3, 2019

ah, sphinx syntax error...

Copy link
Member

@emcastillo emcastillo left a comment

Choose a reason for hiding this comment

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

LGTM

@emcastillo
Copy link
Member

Jenkins, test this please

@crcrpar
Copy link
Contributor Author

crcrpar commented Oct 7, 2019

Ah...
I'll fix this kind of failures once GPUs available


2019-10-07 10:46:34.260067 STDOUT 2327] | =================================== FAILURES ===================================
-- | --
2019-10-07 10:46:34.260069 STDOUT 2327] | _ TestConvolution2D_use_chainerx_false__chainerx_device_None__use_cuda_true__cuda_device_1__use_cudnn_always__cudnn_deterministic_false__autotune_false__cudnn_fast_batch_normalization_false__use_ideep_never_param_0_{W_dtype=float16, x_dtype=float16}.test_from_params _
2019-10-07 10:46:34.260334 STDOUT 2327] |  
2019-10-07 10:46:34.260336 STDOUT 2327] | self = <chainer.testing._bundle.TestConvolution2D_use_chainerx_false__chainerx_device_None__use_cuda_true__cuda_device_1__use...fast_batch_normalization_false__use_ideep_never_param_0_{W_dtype=float16, x_dtype=float16} testMethod=test_from_params>
2019-10-07 10:46:34.260339 STDOUT 2327] | backend_config = <BackendConfig use_chainerx=False chainerx_device=None use_cuda=True cuda_device=1 use_cudnn='always' cudnn_deterministic=False autotune=False cudnn_fast_batch_normalization=False use_ideep='never'>
2019-10-07 10:46:34.260342 STDOUT 2327] |  
2019-10-07 10:46:34.260343 STDOUT 2327] | def test_from_params(self, backend_config):
2019-10-07 10:46:34.260344 STDOUT 2327] | link1 = self.create_link(self.generate_params())
2019-10-07 10:46:34.260346 STDOUT 2327] | link1.to_device(backend_config.device)
2019-10-07 10:46:34.260347 STDOUT 2327] | link2 = links.Convolution2D.from_params(
2019-10-07 10:46:34.260348 STDOUT 2327] | >           link1.W, link1.b, stride=self.stride, pad=self.pad)
2019-10-07 10:46:34.260350 STDOUT 2327] |  
2019-10-07 10:46:34.260351 STDOUT 2327] | chainer/tests/chainer_tests/links_tests/connection_tests/test_convolution_2d.py:118:
2019-10-07 10:46:34.260352 STDOUT 2327] | _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
2019-10-07 10:46:34.260353 STDOUT 2327] | usr/local/lib/python3.5/dist-packages/chainer/links/connection/convolution_2d.py:214: in from_params
2019-10-07 10:46:34.260355 STDOUT 2327] | dilate=dilate, groups=groups)
2019-10-07 10:46:34.260356 STDOUT 2327] | usr/local/lib/python3.5/dist-packages/chainer/links/connection/convolution_2d.py:134: in __init__
2019-10-07 10:46:34.260575 STDOUT 2327] | self._initialize_params(in_channels)
2019-10-07 10:46:34.260578 STDOUT 2327] | usr/local/lib/python3.5/dist-packages/chainer/links/connection/convolution_2d.py:168: in _initialize_params
2019-10-07 10:46:34.260579 STDOUT 2327] | self.W.initialize(W_shape)
2019-10-07 10:46:34.260581 STDOUT 2327] | usr/local/lib/python3.5/dist-packages/chainer/variable.py:1799: in initialize
2019-10-07 10:46:34.260582 STDOUT 2327] | self.initializer, shape, xp, device=device)
2019-10-07 10:46:34.260584 STDOUT 2327] | usr/local/lib/python3.5/dist-packages/chainer/initializers/__init__.py:69: in generate_array
2019-10-07 10:46:34.260585 STDOUT 2327] | initializer(array)
2019-10-07 10:46:34.260586 STDOUT 2327] | usr/local/lib/python3.5/dist-packages/chainer/initializers/constant.py:58: in __call__
2019-10-07 10:46:34.260587 STDOUT 2327] | array, self.fill_value.astype(array.dtype, copy=False))
2019-10-07 10:46:34.260588 STDOUT 2327] | cupy/core/core.pyx:318: in cupy.core.core.ndarray.astype
2019-10-07 10:46:34.260590 STDOUT 2327] | ???
2019-10-07 10:46:34.260598 STDOUT 2327] | cupy/core/core.pyx:389: in cupy.core.core.ndarray.astype
2019-10-07 10:46:34.260600 STDOUT 2327] | ???
2019-10-07 10:46:34.260601 STDOUT 2327] | cupy/core/_kernel.pyx:828: in cupy.core._kernel.ufunc.__call__
2019-10-07 10:46:34.260602 STDOUT 2327] | ???


@chainer-ci
Copy link
Member

Jenkins CI test (for commit 9ea1ca8, target branch master) failed with status FAILURE.

@emcastillo
Copy link
Member

Jenkins, test this please

@chainer-ci
Copy link
Member

Jenkins CI test (for commit 8566494, target branch master) succeeded!

@emcastillo emcastillo added this to the v7.0.0 milestone Oct 31, 2019
@emcastillo emcastillo added st:test-and-merge State indicating that pull request is approved by a reviewer and can be merged after CI passes. and removed st:awaiting-author State indicating that response is needed from contributors, often authors of pull request. labels Oct 31, 2019
@mergify mergify bot merged commit e189205 into chainer:master Oct 31, 2019
@emcastillo emcastillo changed the title [RFC] Add from_params to Linear & Conv Add from_params to Linear & Conv Dec 5, 2019
@chainer-ci
Copy link
Member

Jenkins CI test (for commit 8566494, target branch master) succeeded!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cat:enhancement Implementation that does not break interfaces. st:test-and-merge State indicating that pull request is approved by a reviewer and can be merged after CI passes.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants