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

Support default dtype in F.spatial_transformer_grid #5114

Merged
merged 7 commits into from Aug 29, 2018
Merged

Support default dtype in F.spatial_transformer_grid #5114

merged 7 commits into from Aug 29, 2018

Conversation

takagi
Copy link
Member

@takagi takagi commented Jul 18, 2018

This PR is a part of #4582, makes F.spatial_transformer_gird use the default dtype.

@takagi takagi added the cat:feature Implementation that introduces new interfaces. label Jul 18, 2018
@takagi takagi force-pushed the dtype-spatial-transformer-grid branch from 6098259 to 8520bff Compare July 18, 2018 06:43
@beam2d beam2d mentioned this pull request Jul 18, 2018
Closed
36 tasks
@takagi takagi force-pushed the dtype-spatial-transformer-grid branch from 8520bff to 97fd517 Compare July 18, 2018 13:38
beam2d
beam2d reviewed Jul 20, 2018
View reviewed changes
Copy link
Member

@beam2d beam2d left a comment

Choose a reason for hiding this comment

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

LGTM except one comment.

tests/chainer_tests/functions_tests/array_tests/test_spatial_transformer_grid.py Outdated
class TestSpatialTransformerGrid(unittest.TestCase):

def setUp(self):
self._old_dtype = None
Copy link
Member

Choose a reason for hiding this comment

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

How about using using_config by calling __enter__ and __exit__ directly?

    def setUp(self):
        self._config_user = chainer.using_config('dtype', self.dtype)
        self._config_user.__enter__()
        ...
    def tearDown(self):
        self._config_user.__exit__(None, None, None)

Copy link
Member

Choose a reason for hiding this comment

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

(I added another comment below; that has higher priority so please look that first!)

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed!

beam2d
beam2d reviewed Jul 20, 2018
View reviewed changes
chainer/functions/array/spatial_transformer_grid.py Outdated
@@ -16,14 +16,15 @@ class SpatialTransformerGrid(function.Function):

def __init__(self, output_shape):
self.output_shape = output_shape
self._dtype = chainer.get_dtype()
Copy link
Member

Choose a reason for hiding this comment

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

I overlooked it; I think it's better to accept theta of any dtype with kind f like many other functions do instead of restricting it to the current dtype.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed!

This was referenced Jul 20, 2018
Merged
Merged
Merged
@takagi takagi mentioned this pull request Jul 20, 2018
Closed
beam2d
beam2d reviewed Jul 23, 2018
View reviewed changes
Copy link
Member

@beam2d beam2d left a comment

Choose a reason for hiding this comment

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

Added one comment.

chainer/functions/array/spatial_transformer_grid.py Outdated
xp.linspace(-1, 1, H, dtype=numpy.float32),
xp.linspace(-1, 1, W, dtype=numpy.float32), indexing='ij',
xp.linspace(-1, 1, H, dtype=self._dtype),
xp.linspace(-1, 1, W, dtype=self._dtype), indexing='ij',
Copy link
Member

Choose a reason for hiding this comment

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

Use theta.dtype instead of self._dtype? (ditto for other parts, too.) We could remove self._dtype, too.

@kmaehashi
Copy link
Member

@takagi ping?

@kmaehashi kmaehashi closed this Aug 14, 2018
@kmaehashi kmaehashi reopened this Aug 14, 2018
@kmaehashi kmaehashi added the st:awaiting-author State indicating that response is needed from contributors, often authors of pull request. label Aug 14, 2018
@takagi
Copy link
Member Author

takagi commented Aug 16, 2018

Now I touch this.

@takagi
Copy link
Member Author

takagi commented Aug 16, 2018

Jenkins, test this please.

@takagi takagi removed the st:awaiting-author State indicating that response is needed from contributors, often authors of pull request. label Aug 16, 2018
@chainer-ci
Copy link
Member

Jenkins CI test (for commit f5aa400, target branch master) failed with status FAILURE.
(For contributors, please wait until the reviewer confirms the details of the error.)

@takagi
Copy link
Member Author

takagi commented Aug 17, 2018

Jenkins, test this please.

@chainer-ci
Copy link
Member

Jenkins CI test (for commit c882d63, target branch master) failed with status FAILURE.
(For contributors, please wait until the reviewer confirms the details of the error.)

@okuta okuta self-assigned this Aug 29, 2018
@okuta okuta added this to the v5.0.0b5 milestone Aug 29, 2018
@okuta
Copy link
Member

okuta commented Aug 29, 2018

jenkins, test this please.

@chainer-ci
Copy link
Member

Jenkins CI test (for commit c882d63, target branch master) failed with status FAILURE.
(For contributors, please wait until the reviewer confirms the details of the error.)

@okuta
Copy link
Member

okuta commented Aug 29, 2018

LGTM!

@okuta okuta merged commit 1e23393 into chainer:master Aug 29, 2018
@takagi takagi added to-be-backported Pull request that should be backported. and removed to-be-backported Pull request that should be backported. labels Jan 16, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@beam2d beam2d beam2d left review comments

Assignees

@okuta okuta

Labels
cat:feature Implementation that introduces new interfaces.
Projects
None yet
Milestone
v5.0.0rc1
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@takagi @kmaehashi @chainer-ci @okuta @beam2d