Skip to content

Conversation

@mfeurer
Copy link
Collaborator

@mfeurer mfeurer commented Apr 20, 2018

This PR fixes a few unit tests and adds one which I forgot to check in quite a while ago. Most importantly, the return values to retrieve all runs for a combination of task/setup is now consistent. Furthermore, I think cleaning the test server from lots of runs was really important.

for did in datasets:
self._check_dataset(datasets[did])

# TODO implement these tests
Copy link
Member

Choose a reason for hiding this comment

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

Sure you didn't forget these?

Copy link
Member

@ArlindKadra ArlindKadra Apr 20, 2018

Choose a reason for hiding this comment

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

@janvanrijn you mean we should not verify the datasets?

Copy link
Member

Choose a reason for hiding this comment

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

I mean there's still a TODO in the PR ;)

(line 16)

Copy link
Member

Choose a reason for hiding this comment

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

Ah you are right, I did not see what you meant because of the lines selected. The TODO seems already implemented.

downloaded_flow = openml.flows.get_flow(flow_exists)
setup_exists = openml.setups.setup_exists(downloaded_flow)
self.assertIsInstance(setup_exists, int)
self.assertGreater(setup_exists, 0)
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we can just use assertGreater instead of also making use of assertIsInstance, since if the argument is False it will throw AttributeError: 'NoneType' object has no attribute 'fail'.

@mfeurer mfeurer merged commit fdd6c25 into develop Apr 21, 2018
@mfeurer mfeurer deleted the fix/unittests branch April 21, 2018 13:31
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.

4 participants