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

UnboundLocalError if input is a Dataframe #8290

Merged
merged 1 commit into from Nov 14, 2017
Merged

UnboundLocalError if input is a Dataframe #8290

merged 1 commit into from Nov 14, 2017

Conversation

native-api
Copy link
Contributor

@fchollet
Copy link
Member

Please add a corresponding unit test.

@native-api
Copy link
Contributor Author

I can't seem find a use case for the test. Whenever I pass a dataframe, it fails a dimension check. Looks like inputs and outputs are supposed to be an array of 2D arrays i.e. 3D. But Pandas dataframes can only be 2D.

@fchollet
Copy link
Member

fchollet commented Nov 6, 2017

This test includes testing Pandas dataframes as model inputs: https://github.com/fchollet/keras/blob/master/tests/keras/engine/test_training.py#L94

@native-api
Copy link
Contributor Author

native-api commented Nov 6, 2017 via email

@fchollet
Copy link
Member

fchollet commented Nov 8, 2017

If the current tests are insufficient, please add proper tests. All changes to the codebase should be tested.

@AshuJodel
Copy link

is it merged now?

Copy link

@AshuJodel AshuJodel left a comment

Choose a reason for hiding this comment

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

yes, it is right.

@native-api
Copy link
Contributor Author

@icyblade , what use cases did you have in mind when you wrote that chunk of code?
I could really use a nudge at this point.

@icyblade
Copy link
Contributor

@native-api Thanks for your contribution. It seems to be a typo of my PR #8199, and as the unit test of my PR didn't cover everything, this bug was merged into master. My bad :-(

@fchollet Should I create a new PR including the typo fix and new unit test, or instruct @native-api in this PR?

@fchollet
Copy link
Member

@icyblade: please open a new PR to add unit tests. I'll merge this one.

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

4 participants