Skip to content

Conversation

@artidoro
Copy link
Contributor

@artidoro artidoro commented Dec 7, 2018

Fixes #1791.

As this is a WIP PR, I am still completing the work, but I would really appreciate your feedback on what I have done so far.

In this PR:

  • I created a new ITransformer for FeatureContributionCalculation (previously known as WhatTheFeature) by converting previous ISchemaBoundRowMapper to two separate classes: a IRowMapper and a ISchemaBoundMapper.
  • I created a new IEstimator that produces the transformer.
  • I added tests for the Transformer and Estimator. (Need to add a few more)
  • I added MlContext extensions.
  • I added documentation, and checks for arguments and types.

Will do in a separate PR:

  • Add static extensions for the estimators.
  • I allowed pipelines, and not just IPredictor, to be passed to the IEstimator.

@artidoro artidoro added the API Issues pertaining the friendly API label Dec 7, 2018
@artidoro artidoro self-assigned this Dec 7, 2018
Copy link
Contributor

@rogancarr rogancarr left a comment

Choose a reason for hiding this comment

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

I did a non-technical pass over to look at usability. Overall, this looks good! Mostly, I'd like to make sure we can use it in pipelines and that we have tests that cover the options and the implementations. (Although maybe implementation tests can be a separate PR.)

@artidoro artidoro force-pushed the fct branch 2 times, most recently from f969628 to 74d6581 Compare December 10, 2018 23:08
@Ivanidzo4ka
Copy link
Contributor

Ivanidzo4ka commented Dec 11, 2018

            Console.WriteLine("{0:0.00}\t{1:0.00}\t{2}\t{3:0.00}\t{4:0.00}\t{5:0.00}\t{6:0.00}",

I think we tend to put results of output into comments for the file. #Resolved


Refers to: docs/samples/Microsoft.ML.Samples/Dynamic/FeatureContributionCalculationTransform.cs:87 in a62a510. [](commit_id = a62a510, deletion_comment = False)

@artidoro artidoro merged commit 337cc55 into dotnet:master Dec 20, 2018
@artidoro artidoro deleted the fct branch January 5, 2019 00:01
@LittleLittleCloud LittleLittleCloud mentioned this pull request Jun 24, 2021
4 tasks
@ghost ghost locked as resolved and limited conversation to collaborators Mar 26, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

API Issues pertaining the friendly API

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants