added y argument to fit methods #59
Conversation
added optional y argument to fit methods of TransformerPipeline and DataFrameMapper
|
I like this! :) It's simple enough, doesn't break existing code (I believe) and adds interesting functionality. As you mentioned in #58, we should add some more tests that exercise this new functionality, as well as updating README documentation. Can you do this? If you need any pointers I'd be happy to help. :) Thanks! |
|
Yes, I'll do this. Let me work on it during the week and see how far I get. Do you have a Python 2 environment you can test the code in when it's ready? I don't have one and it would be nice if I didn't need to set one up. Thanks. |
|
Sure, we use tox to test everything on Python 2 and 3. 2016-07-10 22:46 GMT+02:00 Vitaley Zaretskey notifications@github.com:
|
refactored pipeline and dataframe_mapper to use _call_fit helper function, consolidating code to handle fit with 1 or 2 arguments
added unit tests for _call_fit to test_pipeline
added unit tests to test_dataframe_mapper and updated README
|
I added unit tests for the new functionality and updated the README. Everything is passing on Python 3. Let me know if you see any issues or anything else that needs to be added. Thanks. |
| [ 0., 1., 0., 3., 1.], | ||
| [ 1., 0., 0., 5., -0.], | ||
| [ 0., 0., 1., 4., -1.]]) | ||
| >>> np.round(mapper4.fit_transform(data.copy()), 1) |
There was a problem hiding this comment.
Why does one need to pass "1" as second argument to this transform, and why is the output different from the previous case in the last column?
There was a problem hiding this comment.
The "1" is the second argument of np.round. This test case was failing even though I don't think I modified anything that affected it. The issue seems to be that on my machine np.round(-0.3) equals "0.", not "-0." Changing it to round to 1 decimal place fixed the test case.
|
My apologies for the delay reviewing this. Good work! Tests pass both on Python 2.7 and 3.5. Merging once you've addressed the inline comments I did on the existing commits. |
removed unused import
|
Thanks. I answered the comments and made a new commit. @dukebody |
|
All tests passing, so merged! |
Added optional y argument to fit methods of TransformerPipeline and DataFrameMapper. I'm using try/except TypeError to handle fit methods of pipeline transformers that don't accept a y argument. @dukebody