Skip to content

Fix636 - #637

Merged
PGijsbers merged 9 commits into
developfrom
fix636
Mar 6, 2019
Merged

Fix636#637
PGijsbers merged 9 commits into
developfrom
fix636

Conversation

@PGijsbers

@PGijsbersPGijsbers commented Mar 4, 2019

Copy link
Copy Markdown
Collaborator

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
Copy Markdown
CollaboratorAuthor

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
Copy Markdown
CollaboratorAuthor

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
Copy Markdown
Collaborator

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

janvanrijn commented Mar 4, 2019 via email

Copy link
Copy Markdown
Member

@PGijsbers

Copy link
Copy Markdown
CollaboratorAuthor

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

@codecov-io

codecov-io commented Mar 5, 2019

Copy link
Copy Markdown

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 FilesCoverage Δ
openml/flows/sklearn_converter.py90.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 mfeurerMarch 5, 2019 15:04
Comment threadtests/test_flows/test_flow_functions.py
Comment threadopenml/flows/sklearn_converter.py Outdated
@PGijsbers
PGijsbers requested a review from mfeurerMarch 5, 2019 19:55
Comment threadopenml/flows/sklearn_converter.py
@PGijsbers
PGijsbers merged commit 9c74931 into developMar 6, 2019
@PGijsbers
PGijsbers deleted the fix636 branch March 6, 2019 13:50
Sign up for freeto 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.

4 participants

@PGijsbers@mfeurer@janvanrijn@codecov-io