-
Notifications
You must be signed in to change notification settings - Fork 834
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
Outlier example #1306
Outlier example #1306
Conversation
Check out this pull request on You'll be able to see Jupyter notebook diff and discuss changes. Powered by ReviewNB. |
Fri Dec 27 13:18:53 UTC 2019 impatient try |
Fri Dec 27 13:19:00 UTC 2019 impatient try |
Fri Dec 27 14:41:52 UTC 2019 impatient try |
Fri Dec 27 14:42:01 UTC 2019 impatient try |
There are quite a lot of areas with "Notes from Rafal" which I assume would be removed, but several have content that may be worth keeping - would this be something you're also planning to add in this PR? |
Ou... Sorry about those. They're in the Kubeflow example - I wanted to keep
those notes in separate branch of my fork and had to start working on this
PR with a wrong base by accident.
Please disregard this first commit for now, I will rebase on top of master
once I'm back leaving only relevant changes here.
BTW, what do you think about the two approaches to the actual outliers
example, which one you think is better?
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice one! Looks solid, I just added a few comments, would be good to hear your thoughts
examples/outliers/alibi-detect-combiner/pipeline/combiner/Combiner.py
Outdated
Show resolved
Hide resolved
examples/outliers/alibi-detect-combiner/pipeline/combiner/Combiner.py
Outdated
Show resolved
Hide resolved
examples/outliers/alibi-detect-combiner/pipeline/outliersdetector/Detector.py
Outdated
Show resolved
Hide resolved
Got it, sounds good. I was going to add a few more comments on the /kubeflow/ dir but I'll skip it In regards to the approaches, I can see in the notebook that the way you show it is through the Model wrapper + remote_detect_fn, but you also include the method with the combiner. I think seeing the code for the combiner, it actually seems more intuitive in regards to the separation of concerns, as you actually build the wrapper for the detector and model separately and then use the combiner to just aggregate them. Maybe it would actually be best to go for the latter one, and providing a bit of an intro to the concept of the combiner. Looks solid! |
Glad to hear! I will cleanup the first approach and rebase on current master. |
Thu Jan 2 13:35:18 UTC 2020 impatient try |
Thu Jan 2 13:35:24 UTC 2020 impatient try |
@axsaucedo Currently the tutorial consists of three This would be similar to spacy tokernizer from Kubeflow example. |
Thu Jan 2 14:49:36 UTC 2020 impatient try |
Thu Jan 2 14:49:48 UTC 2020 impatient try |
Thu Jan 2 15:30:58 UTC 2020 impatient try |
Thu Jan 2 15:36:22 UTC 2020 impatient try |
I tried it in 6a5a714 and seems to work quite nice! |
Thu Jan 2 15:36:30 UTC 2020 impatient try |
Thu Jan 2 15:58:47 UTC 2020 impatient try |
Thu Jan 2 15:58:52 UTC 2020 impatient try |
Fri Jan 3 14:24:08 UTC 2020 impatient try |
Fri Jan 3 14:24:23 UTC 2020 impatient try |
Fri Jan 3 14:27:55 UTC 2020 impatient try |
Fri Jan 3 14:28:01 UTC 2020 impatient try |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another thing that would still be required is to add the reference in the docs so it actually appears in the documentation. For this you will have to add an nblink file with a reference to the notebook, and a link to the nblink file in the examples page.
Done! |
Tue Jan 7 16:00:27 UTC 2020 impatient try |
Tue Jan 7 16:00:38 UTC 2020 impatient try |
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: axsaucedo The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
New example of seldon deployment of loan classifier with outlier detection.
This is still WIP. Right now there are two approaches to this tutorial:first with all code defined in notebook that uses%%writefile
to populate python modulescombiner
in seldon graphI am keen to go with the second approach as it seems easier to navigate, reader is less overwhelmed by lengthy notebook and code is easier to version control and reuse.
What's missing: