Skip to content

Fix636 #637

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.

Sign up for GitHub

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

Jump to bottom
Merged
PGijsbers merged 9 commits into from
Mar 6, 2019
Merged

Fix636 #637

PGijsbers merged 9 commits into from
Mar 6, 2019

Conversation

@PGijsbers
Copy link
Collaborator

@PGijsbers PGijsbers commented Mar 4, 2019 •
edited
Loading

Fixes an issue where a list of lists with simple types could not be serialized (because it was expected to be FeatureUnion, Pipeline or VotingClassifier), as described in #636.

Example flow generated: https://www.openml.org/f/9556
It deserializes correctly with

flow = openml.flows.get_flow(9556)
model = openml.flows.flow_to_sklearn(flow)

@PGijsbers
Copy link
Collaborator Author

PGijsbers commented Mar 4, 2019

The unit test uses OrdinalEncoder which is only available in sklearn >= 0.20, which I forgot about. I will switch in an older scikit-learn component.

@PGijsbers
Copy link
Collaborator Author

PGijsbers commented Mar 4, 2019

I can't find a scikit-learn component in 0.19 or 0.18 which accepts a list of lists. Perhaps I overlooked one, can anyone think of any? If not, would it be fine to only test this against scikit-learn>=0.20?

@mfeurer
Copy link
Collaborator

mfeurer commented Mar 4, 2019

I'm not aware of one either. Unless @amueller or @janvanrijn now of one I think you can simply skip the tests for those sklearn versions.

@janvanrijn
Copy link
Member

janvanrijn commented Mar 4, 2019 via email

@PGijsbers
Copy link
Collaborator Author

PGijsbers commented Mar 4, 2019

@janvanrijn thanks for the suggestion, unfortunately it looks like you just specify layer sizes through a tuple of n_layers integers.

@codecov-io
Copy link

codecov-io commented Mar 5, 2019 •
edited
Loading

Codecov Report

Merging #637 into develop will increase coverage by 0.02%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #637      +/-   ##
===========================================
+ Coverage    89.79%   89.81%   +0.02%     
===========================================
  Files           32       32              
  Lines         3313     3320       +7     
===========================================
+ Hits          2975     2982       +7     
  Misses         338      338
Impacted Files Coverage Δ
openml/flows/sklearn_converter.py 90.65% <100%> (+0.14%) ⬆️

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 0235c51...b98987e. Read the comment docs.

@PGijsbers PGijsbers requested a review from mfeurer March 5, 2019 15:04
mfeurer
mfeurer requested changes Mar 5, 2019
View reviewed changes
@PGijsbers PGijsbers requested a review from mfeurer March 5, 2019 19:55
mfeurer
mfeurer reviewed Mar 6, 2019
View reviewed changes
@PGijsbers PGijsbers merged commit 9c74931 into develop Mar 6, 2019
@PGijsbers PGijsbers deleted the fix636 branch March 6, 2019 13:50
@PGijsbers PGijsbers mentioned this pull request Mar 6, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mfeurer mfeurer mfeurer approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@PGijsbers @mfeurer @janvanrijn @codecov-io