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

theano.tensor.constant reshape fix #3323

Merged
merged 6 commits into from Aug 27, 2015
Merged

Conversation

SinaHonari
Copy link
Contributor

fixes issue #3031

raise TypeError("Shape must be integers", shp, shp.dtype)
assert shp.ndim == 1
if not(isinstance(shp, TensorConstant) and not shp.data.size == 0):
assert shp.ndim == 1
Copy link
Member

Choose a reason for hiding this comment

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

I was wrong, I verified and as_tensor_variable(()).ndim == 1. You don't need to modify this.

@bouthilx
Copy link
Member

@SinaHonari
Thanks for the quick PR!

You still need to add a test case to ensure this bug does not happen again. Add a new method test_empty_shp() at the end of this class

@SinaHonari
Copy link
Contributor Author

Boss your comments and concerns are all realized :)

@bouthilx
Copy link
Member

Thanks Sina! But you forgot about the unit-test. ;)

@SinaHonari
Copy link
Contributor Author

What should the unit test do?

@bouthilx
Copy link
Member

Test the basic case reported and make sure it doesn't fail. #3031

theano.tensor.constant([1]).reshape(())

As I said above

You still need to add a test case to ensure this bug does not happen again. Add a new method test_empty_shp() at the end of this class.

Should be quite simple. :)

@SinaHonari
Copy link
Contributor Author

You can check if the test is sufficient.

@bouthilx
Copy link
Member

Please compile the function and verify that output.shape == ()
That will be sufficient. :)

@bouthilx
Copy link
Member

Looks good to me, Thanks Sina!

bouthilx added a commit that referenced this pull request Aug 27, 2015
theano.tensor.constant reshape fix
@bouthilx bouthilx merged commit 1d13344 into Theano:master Aug 27, 2015
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.

None yet

2 participants