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

Slight fix on entropy test #4424

Closed
wants to merge 2 commits into from
Closed

Conversation

takagi
Copy link
Member

@takagi takagi commented Dec 8, 2020

This PR follows #4369 to slightly fix its test.

@takagi takagi added the cat:test Test code / CI label Dec 8, 2020
'default': 1e-15},
scipy_name='scp')
def test_entropy(self, xp, scp, dtype):
# `entropy` returns float32 value for float16 input so float32
# tolerance is used for it.
Copy link
Member

Choose a reason for hiding this comment

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

Are the intermediate computations done in float32?
If that is the case this is fine. But if all the steps are done in float16, even though the output is float32, there will be precision losses. So the original tolerance should be kept.

Copy link
Contributor

Choose a reason for hiding this comment

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

In SciPy, float16 inputs do not seem to be explicitly cast to float32, so the normalization step that divides by a sum over the array will use float16. Thus, the normalization step here should be more accurate than in SciPy. The later use of the scipy.special functions does result in things becoming float32, though (I don't think any functions in scipy.special support half precision)

@takagi
Copy link
Member Author

takagi commented Dec 9, 2020 via email

@takagi
Copy link
Member Author

takagi commented Dec 9, 2020 via email

@takagi
Copy link
Member Author

takagi commented Dec 9, 2020

Shuffle test failed as axis parameter is supported since scipy==1.4.0.

@emcastillo
Copy link
Member

Jenkins, test this please

@chainer-ci
Copy link
Member

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

@emcastillo
Copy link
Member

Seems like it didn't go well 😁

@toslunar
Copy link
Member

Could you merge #4481 first?

@emcastillo
Copy link
Member

Jenkins, test this please

@chainer-ci
Copy link
Member

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

@takagi takagi closed this Jan 8, 2021
@takagi takagi deleted the comment-entropy-test branch January 8, 2021 02:52
@asi1024 asi1024 added this to the Closed PRs milestone Jan 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants