Uh oh!
There was an error while loading. Please reload this page.
Conversation
janvanrijn
commented
Oct 6, 2018
Btw, the following testcases fail because they are ran against the live server. These will be fine once this PR is merged. |
janvanrijn
commented
Oct 7, 2018
Codecov Report
@@ Coverage Diff @@## develop #567 +/- ##
===========================================
- Coverage 90.09% 90.08% -0.01%
===========================================
Files 32 32 Lines 2999 3118 +119 ===========================================
+ Hits 2702 2809 +107 - Misses 297 309 +12
Continue to review full report at Codecov.
|
…class. updated unit tests accordingly
janvanrijn
commented
Oct 10, 2018
This PR now adds the functionality that is required for #564. In order to do this off we need more information about the flow structure. For this, I added the functions With these functionalities, solving #564 is rather easy. For this I added a function to the sklearn converter (in setups, but maybe we want to pull these in the flow sklearn converter later). This function now provides a convenience function that maps the name of the OpenMLParameter to the name of the sklearn parameter (e.g., if the openml name is Ready for review @mfeurer@ArlindKadra |
mfeurer
commented
Oct 23, 2018
Could you please add an example on how to use this to the docs so it is more clear to potential users? |
janvanrijn
commented
Dec 5, 2018
I know that you asked for an example, but I found it hard to construct a general usable example. In the process, I did come up with a general (run setup) tutorial, and I used it to simplify and remove a complex part of the setup reinstantiation code. Hope this clarifies this PR a bit. Let me know if you have further questions |
janvanrijn
commented
Dec 5, 2018
To add to this, here is the function where I used this: It is to transform an OpenML setup object into a sklearn parameter dict, something that is quite hard without these convenience functions. This is what I wrote in the tutorial, before I realized that the tutorial is obsolute due to easier functions: |
mfeurer
left a comment
There was a problem hiding this comment.
Hey, I think I got the functions and they make sense. I only have a few comments.
Uh oh!
There was an error while loading. Please reload this page.
| use them) | ||
| A key requirement for reinstantiating a flow is to have the same scikit-learn | ||
| version as the flow that was uploaded. This tutorial will upload the flow |
There was a problem hiding this comment.
Don't these two sentences contradict each other?
There was a problem hiding this comment.
Sorry, I also don't get your updated version. You're saying it is important to have the same scikit-learn version, but then say this doesn't matter because the tutorial uploads the flow. I simply don't get how this makes the version not matter.
There was a problem hiding this comment.
I tried to reformulate. Please let me know if this is any better.
Uh oh!
There was an error while loading. Please reload this page.
| and solve the same task again. | ||
| 3) We will verify that the obtained results are exactly the same. | ||
| Readers interested in reinstantiating a setup can skip part 1 and 2 and start | ||
| with part 3 immediately. |
There was a problem hiding this comment.
Doesn't part 3 only compare the two arrays?
There was a problem hiding this comment.
Yes, but is is quite an important point and it combines pt 1 and pt 2. I would prefer to keep it in a separate part, albeit only one line of code (it could be extended with more checks later, if someone is motivated)
There was a problem hiding this comment.
I agree on that. What I wanted to point out is that the sentence says:
Readers interested in reinstantiating a setup can skip part 1 and 2 and start with part 3 immediately.
which to me reads like "if you're interested in 1 and 2 you can skip them and commence to 3" because 2 is about reinstantiating the flow, right?
There was a problem hiding this comment.
You are right. I removed the sentence.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
janvanrijn
commented
Dec 10, 2018
ping @mfeurer |
janvanrijn
commented
Dec 11, 2018
Awesome! |
Maybe, let's solve #564 in two separate PR's, too keep the PR small and manageable? For #564 to happen, we need to have some knowledge about the structure of the scikit-learn flow. This is all in the OpenML flow object, but in a hard to extract format. I added a function
flow_format, that extracts all needed information from the scikit-learn flow. Nice byproduct is that the result of this function integrates nicely with unit test functions. I integrated several, and can add more.