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

don't add additional classes if FailureModel encountered #1984

Merged
merged 8 commits into from May 23, 2019

Conversation

Projects
None yet
3 participants
@dagola
Copy link
Contributor

commented Aug 21, 2017

fixes #1981. I've run devtools::test(filter = "base") and it doesn't seem to introduce new errors. The code I've posted in #1981 does show my expected behaviour now.

However, I tried to write a test to replicate the behaviour of #1981 using the mock learners, but I have to admit, that I'm not able to do that. Unfortunately, I don't have the time at the moment to dive deeper in the mlr internals to add a corresponding test - my apologies for that.

@ja-thomas
Copy link
Member

left a comment

Hi,

two things:

  • Can you rebase on the mlr master branch so that Travis can run
  • Can you add an unit test for this?
@dagola

This comment has been minimized.

Copy link
Contributor Author

commented Sep 12, 2017

Rebase complete.

I will try to figure out how to create a test that mimics #1981 in the next week. Any tips? Is the use of mock learners appropriate?

@ja-thomas

This comment has been minimized.

Copy link
Member

commented Sep 14, 2017

Cool.

Try to keep the unit test as small as possible, i.e., reduce your initial code from the bug report as much as possible. And use a mock learner to trigger a failureModel.

When you have an initial version for the unittest we can review.

Thanks

EDIT: I think your rebase didn't quite work, there are a lot of changes that are not relevant for your PR

@dagola

This comment has been minimized.

Copy link
Contributor Author

commented Oct 5, 2017

Sorry for the delay, I've added a test now.

Probably I messed up the rebasing, because I don't really understand what to do. I'll try to fix.

@dagola dagola force-pushed the dagola:fix_failure_model branch from bb54ccb to 2e0c899 Oct 5, 2017

@dagola

This comment has been minimized.

Copy link
Contributor Author

commented Oct 6, 2017

@ja-thomas Can you assist me on rebasing, please? Should I choose a specific commit from master or just do git rebase master? If I run the latter, all commits on master until now are included but that's not what you want me to do, I guess?

@ja-thomas

This comment has been minimized.

Copy link
Member

commented Oct 9, 2017

Since you're working in a seperate fork I can't directly do the rebase for you.

The steps should look like that:

This should solve the problem

@dagola

This comment has been minimized.

Copy link
Contributor Author

commented Oct 9, 2017

Thanks for your help. Looks like this is clean now.

@berndbischl berndbischl force-pushed the mlr-org:master branch 2 times, most recently from cf3db65 to 57f099e Mar 3, 2018

@ja-thomas ja-thomas force-pushed the mlr-org:master branch from 57f099e to 20d5a39 Mar 3, 2018

@pat-s

This comment has been minimized.

Copy link
Member

commented Apr 21, 2019

@dagola Could you check the checkbox "allow edits from maintainers"? So we can finish this PR here. Otherwise I would manually need to copy all of your changes to make additional ones.

The Travis error is probably caused because variable measure was not set in the test.

@dagola

This comment has been minimized.

Copy link
Contributor Author

commented Apr 23, 2019

@pat-s The checkbox is checked.
Screenshot 2019-04-23 at 08 02 30

dagola and others added some commits Apr 23, 2019

@pat-s

This comment has been minimized.

Copy link
Member

commented May 19, 2019

Thanks. Merging soon. Thanks for contributing!

@pat-s pat-s merged commit b6f830f into mlr-org:master May 23, 2019

1 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
deploy/netlify Deploy preview ready!
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.