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

Single input task partial fix #541

Merged
merged 7 commits into from Dec 3, 2018
Merged

Single input task partial fix #541

merged 7 commits into from Dec 3, 2018

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.

@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

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.

@@ -156,6 +156,11 @@ def test_get_task_with_cache(self):
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.

None yet

3 participants