Skip to content

Conversation

@ArlindKadra
Copy link
Member

@ArlindKadra ArlindKadra commented Sep 20, 2018

Reference Issue

#538

What does this PR implement/fix? Explain your changes.

Handling cases where tasks have only one input. Change to the cache dir.

How should this PR be tested?

Added unit test for get_task with different task types.

@ArlindKadra ArlindKadra changed the title Partial starting fix for single input task, cache dir multiplatform c… Single input task partial fix Sep 20, 2018
@codecov-io
Copy link

codecov-io commented Sep 20, 2018

Codecov Report

❗ No coverage uploaded for pull request base (develop@b9035c4). Click here to learn what that means.
The diff coverage is 72.72%.

Impacted file tree graph

@@            Coverage Diff             @@
##             develop     #541   +/-   ##
==========================================
  Coverage           ?   89.82%           
==========================================
  Files              ?       32           
  Lines              ?     2920           
  Branches           ?        0           
==========================================
  Hits               ?     2623           
  Misses             ?      297           
  Partials           ?        0
Impacted Files Coverage Δ
openml/config.py 90.19% <100%> (ø)
openml/tasks/functions.py 86.39% <62.5%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b9035c4...6103c02. Read the comment docs.

@ArlindKadra ArlindKadra requested a review from mfeurer September 20, 2018 23:04
@mfeurer
Copy link
Collaborator

mfeurer commented Sep 24, 2018

@ArlindKadra could you please rebase this on the development branch and update for the fact that we have multiple task classes by now?

if not isinstance(task, OpenMLClusteringTask):
task.class_labels = \
dataset.retrieve_class_labels(task.target_name)
task.download_split()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would it make sense to move this into the task classes? If they have a split and class labels they retrieve them, otherwise, they don't. Also, how does this work for regression tasks (regarding class labels)?

Copy link
Member Author

@ArlindKadra ArlindKadra Nov 30, 2018

Choose a reason for hiding this comment

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

Nice point, it does not work for regression. What do you think about this.
As for your first point, I do not really like it, because then we have to call get_dataset from the task.

if isinstance(task, OpenMLSupervisedTask):
    task.download_split()
    if isinstance(task, OpenMLClassificationTask): 
        task.class_labels = \
            dataset.retrieve_class_labels(task.target_name)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Agreed. Your proposed solution is fine for this.

task = openml.tasks.get_task(1)
self.assertIsInstance(task, OpenMLTask)

def test_get_task_clustering(self):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you please add a unit test for regression, too.

Copy link
Member Author

Choose a reason for hiding this comment

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

yes I can

@mfeurer mfeurer merged commit 57d61c4 into develop Dec 3, 2018
@mfeurer mfeurer deleted the fix538 branch December 3, 2018 10:22
@mfeurer mfeurer mentioned this pull request Dec 3, 2018
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