Skip to content

Conversion of Multi Class Naive Bayes classifier to estimator #1111

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

Merged
merged 6 commits into from
Oct 4, 2018

Conversation

artidoro
Copy link
Contributor

@artidoro artidoro commented Oct 2, 2018

Ongoing work on converting the trainers to estimators (#754). This PR converts the Multi Class Naive Bayes classification trainer.

@Zruty0 Zruty0 mentioned this pull request Oct 2, 2018
/// <summary>
/// Initializes a new instance of <see cref="MultiClassNaiveBayesTrainer"/>
/// </summary>
/// <param name="env">The private instance of <see cref="IHostEnvironment"/>.</param>
Copy link
Contributor

@Zruty0 Zruty0 Oct 2, 2018

Choose a reason for hiding this comment

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

The private instance of . [](start = 30, length = 55)

this is not helping.
The environment to use. #Resolved

.Concat(MetadataUtils.GetTrainerOutputMetadata()));
return new[]
{
new SchemaShape.Column(DefaultColumnNames.Score, SchemaShape.Column.VectorKind.Vector, NumberType.R4, false, new SchemaShape(MetadataForScoreColumn())),
Copy link
Contributor

@Zruty0 Zruty0 Oct 2, 2018

Choose a reason for hiding this comment

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

MetadataForScoreColumn [](start = 141, length = 22)

  1. why the disparity between this and the line below, where you initialize metadata right in the method?
  2. method names should be verbs. So GetScoreColumnMetadata or something #Resolved

Copy link
Contributor

@Zruty0 Zruty0 left a comment

Choose a reason for hiding this comment

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

:shipit:

var reader = TextLoader.CreateReader(env,
c => (label: c.LoadText(0), features: c.LoadFloat(1, 4)));

MultiClassNaiveBayesPredictor pred = null;
Copy link
Contributor

@Ivanidzo4ka Ivanidzo4ka Oct 2, 2018

Choose a reason for hiding this comment

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

MultiClassNaiveBayesPredictor [](start = 12, length = 29)

Should we expose Label and Feature histograms? Otherwise, I don't see any reason why we actually expose it. #Closed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Created two public methods that will output a copy of the featurehistogram and labelhistogram


In reply to: 222122921 [](ancestors = 222122921)

using Microsoft.ML.Runtime.Internal.Calibration;
using Microsoft.ML.Runtime.Learners;
using Microsoft.ML.StaticPipe.Runtime;

Copy link
Contributor Author

@artidoro artidoro Oct 3, 2018

Choose a reason for hiding this comment

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

change file name to static #Resolved

public void GetLabelHistogram(ref int[] labelHistogram, out int labelCount)
{
labelCount = _labelCount;
Utils.EnsureSize(ref labelHistogram, _labelCount, _labelCount);
Copy link
Contributor

@Ivanidzo4ka Ivanidzo4ka Oct 4, 2018

Choose a reason for hiding this comment

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

_labelCount, _labelCount [](start = 49, length = 24)

Why repeat twice?
Why repeat twice?

We have method which accept only one value, or you deliberately want to shrink array even if it slighlty bigger? #Closed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was thinking of returning exactly the right size for the array, but I see why if we pass a bigger array it might be better not to shrink it.


In reply to: 222538588 [](ancestors = 222538588)

Copy link
Contributor

@Ivanidzo4ka Ivanidzo4ka left a comment

Choose a reason for hiding this comment

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

:shipit:

@artidoro artidoro merged commit 2a4681b into dotnet:master Oct 4, 2018
@artidoro artidoro deleted the nb branch January 5, 2019 00:01
@ghost ghost locked as resolved and limited conversation to collaborators Mar 28, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants