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

ENH: Add some tests for supervised FoM #1126

Merged
merged 1 commit into from Sep 22, 2017
Merged

Conversation

adler-j
Copy link
Member

@adler-j adler-j commented Sep 14, 2017

Adds tests for the most used supervised FoM.

@adler-j
Copy link
Member Author

adler-j commented Sep 21, 2017

Could use a review

Copy link
Member

@kohr-h kohr-h left a comment

Choose a reason for hiding this comment

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

The tests should also check the normalized FOMs. Otherwise good

result = odl.contrib.fom.mean_squared_error(data, true)
expected = np.mean((true - data) ** 2)

assert pytest.approx(result) == expected
Copy link
Member

Choose a reason for hiding this comment

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

Slightly weird to have it this way, I prefer result == pytest.approx(expected)

space = simple_fixture('space', [odl.rn(3),
odl.uniform_discr(0, 1, 10)])

def test_mean_squared_error(space):
Copy link
Member

Choose a reason for hiding this comment

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

Given the recent issue #1136 an additional check that the normalized variants do the right thing would be good. That means checking 0 <= result_normalized <= 1 for random inputs, (x, x) and (x, -x) which are often best and worst case, resp.

@adler-j
Copy link
Member Author

adler-j commented Sep 22, 2017

Merging after CI

@adler-j adler-j merged commit c683fbd into odlgroup:master Sep 22, 2017
@adler-j adler-j deleted the fom_tests branch September 22, 2017 08:34
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