Skip to content

Fix matrix factorization trainer's doc based on user feedback #3170

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Apr 5, 2019
Merged

Fix matrix factorization trainer's doc based on user feedback #3170

merged 1 commit into from
Apr 5, 2019

Conversation

wschin
Copy link
Member

@wschin wschin commented Apr 2, 2019

This PR fixes #3169 by

  • updating key-typed variable's description in matrix factorization trainer's doc, and
  • adding one more reference for alg used.

@wschin wschin self-assigned this Apr 2, 2019
@wschin wschin requested review from artidoro and sfilipi April 2, 2019 16:25
@codecov
Copy link

codecov bot commented Apr 2, 2019 •
edited
Loading

Codecov Report

❗ No coverage uploaded for pull request base (master@fde1ab7). Click here to learn what that means.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master    #3170   +/-   ##
=========================================
  Coverage          ?   72.53%           
=========================================
  Files             ?      807           
  Lines             ?   144774           
  Branches          ?    16208           
=========================================
  Hits              ?   105016           
  Misses            ?    35344           
  Partials          ?     4414
Flag Coverage Δ
#Debug 72.53% <ø> (?)
#production 68.12% <ø> (?)
#test 88.82% <ø> (?)
Impacted Files Coverage Δ
...osoft.ML.Recommender/MatrixFactorizationTrainer.cs 70.39% <ø> (ø)

artidoro
artidoro approved these changes Apr 4, 2019
View reviewed changes
Copy link
Contributor

@artidoro artidoro 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
Copy link
Contributor

artidoro commented Apr 4, 2019 •
edited
Loading 

/// }

To avoid this kind of issues in the future, it might be good to include the code sample that you are giving here in the referenced sample file instead of the trainer's doc.
For maintainability purposes if we check that the samples compile it would be better.

Refers to: src/Microsoft.ML.Recommender/MatrixFactorizationTrainer.cs:50 in 95f09fd. [](commit_id = 95f09fd, deletion_comment = False)

@sfilipi
Copy link
Member

sfilipi commented Apr 5, 2019 •
edited
Loading 

/// }

+1. Yo ucan reference specific lines from a .cs file.

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

Refers to: src/Microsoft.ML.Recommender/MatrixFactorizationTrainer.cs:50 in 95f09fd. [](commit_id = 95f09fd, deletion_comment = False)

sfilipi
sfilipi reviewed Apr 5, 2019
View reviewed changes
src/Microsoft.ML.Recommender/MatrixFactorizationTrainer.cs
/// // Matrix row index starts from firstRowIndex and is at most firstRowIndex+m-1.
/// // Contieuous=true means that all values from firstRowIndex to firstRowIndex+m-1 are allowed keys.
/// [KeyType(Contiguous = true, Count = m, Min = firstRowIndex)]
/// // Matrix column index starts from 0 and is at most n-1.
Copy link
Member

Choose a reason for hiding this comment

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

// Matrix column index starts from 0 and is at most n-1. [](start = 12, length = 56)

tangential, what do you think about #3072

sfilipi
sfilipi reviewed Apr 5, 2019
View reviewed changes
src/Microsoft.ML.Recommender/MatrixFactorizationTrainer.cs
@@ -65,7 +60,7 @@ namespace Microsoft.ML.Trainers
/// <i>R</i> is approximated by the product of <i>P</i>'s transpose and <i>Q</i>. This trainer implements
/// <a href='https://www.csie.ntu.edu.tw/~cjlin/papers/libmf/mf_adaptive_pakdd.pdf'>a stochastic gradient method</a> for finding <i>P</i>
/// and <i>Q</i> via minimizing the distance between<i> R</i> and the product of <i>P</i>'s transpose and Q.</para>.
/// <para>For users interested in the mathematical details, please see the references below.</para>
/// <para>The underlying library used in ML.NET matrix factorization can be found on <a href='https://github.com/cjlin1/libmf'>a Github repository</a>. For users interested in the mathematical details, please see the references below.</para>
Copy link
Member

Choose a reason for hiding this comment

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

on a Github repository [](start = 86, length = 68)

on the libmf Github repository

sfilipi
sfilipi approved these changes Apr 5, 2019
View reviewed changes
Copy link
Member

@sfilipi sfilipi left a comment

Choose a reason for hiding this comment

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

:shipit:

shauheen
shauheen approved these changes Apr 5, 2019
View reviewed changes
Copy link
Contributor

@shauheen shauheen left a comment

Choose a reason for hiding this comment

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

:shipit:

@wschin wschin merged commit 68640bb into dotnet:master Apr 5, 2019
@wschin wschin deleted the fix-mf-doc branch April 5, 2019 18:15
wschin added a commit to wschin/machinelearning that referenced this pull request Apr 11, 2019
@wschin
Update description about key type and add one more reference (dotnet#…
3668b3e 
…3170)
@wschin wschin mentioned this pull request Apr 11, 2019
Closed
@ghost ghost locked as resolved and limited conversation to collaborators Mar 23, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@shauheen shauheen shauheen approved these changes

@artidoro artidoro artidoro approved these changes

@sfilipi sfilipi sfilipi approved these changes

Assignees

@wschin wschin

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

KeyType in example is invalid
4 participants
@wschin @artidoro @sfilipi @shauheen