Skip to content

Conversation

@dataders
Copy link

This is something I did a few weeks ago as I was debugging my own pipeline. For me, this is a a clearer and more robust way to do train test split w/ AutoMLStep and PipelineData objects. The changes I made:

  1. using os.path.join() instead of + to concat strings
  2. testTrainSplitStep has 1 `PipelineData object instead of 4
    @sanpil @yanrez @j-martens let me know what y'all think

@msftclas
Copy link

msftclas commented Aug 14, 2019

CLA assistant check
All CLA requirements met.

Copy link

@purnesh42H purnesh42H left a comment

Choose a reason for hiding this comment

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

Don't include config.json file

delete details
keep the same
Copy link
Author

@dataders dataders left a comment

Choose a reason for hiding this comment

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

I made some commits to make it fit the existing repo. As I understand it, this isn't the real repo though so does getting this approved even matter? I'm just proposing changes to this notebook. @rastala

Copy link

@purnesh42H purnesh42H left a comment

Choose a reason for hiding this comment

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

A check is needed to see if output is specified(not None) as the steps don't enforce it.

@sanpil
Copy link

sanpil commented Aug 15, 2019

@swanderz Please continue to propose changes. Even though the publishing mechanism is different, we could adopt your changes.

@dataders
Copy link
Author

@sanpil this PR is my feedback, can you please give feedback on the version of the notebook I'm proposing (not sure how to get see a helpful diff on .ipynb's in this PR)

Let me know what you think.

If you understand and agree with my changes, I'm then willing to work with @purnesh42H and @rastala to get the PR approved, but we're definitely not at that point yet.

@sanpil
Copy link

sanpil commented Aug 15, 2019

@purnesh42H and I will review the changes and will get back to you. Thanks @swanderz!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants