Skip to content
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

Update outdated codes in cf.md and datasetmapper.md #3398

Merged
merged 9 commits into from Feb 11, 2023
Merged

Update outdated codes in cf.md and datasetmapper.md #3398

merged 9 commits into from Feb 11, 2023

Conversation

AdarshSantoria
Copy link
Contributor

No description provided.

rcurtin
rcurtin reviewed Feb 6, 2023
View reviewed changes
Copy link
Member

@rcurtin rcurtin left a comment

Choose a reason for hiding this comment

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

Thanks again, I love that you are going through all the documentation and finding issues like this. 👍

doc/tutorials/cf.md Outdated
@@ -255,7 +255,7 @@ extern size_t rank;
// Build the CF object and perform the decomposition.
// The constructor takes a default-constructed factorizer, which, by default,
// is of type NMFALSFactorizer.
CF cf(data, NMFALSFactorizer(), neighborhood, rank);
CFType cf(data, NMFPolicy(), neighborhood, rank);
Copy link
Member

Choose a reason for hiding this comment

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

I thought that CF was typedef'ed to CFType<> (the default), but it seems like that code is not actually there in src/mlpack/methods/cf/cf.hpp! Do you want to instead add that typedef, and then this change shouldn't be needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I had added that typedef.

doc/tutorials/datasetmapper.md
std::cout << info.UnmapValue("T", 2) << "\n";
std::cout << info.UnmapValue("F", 2) << "\n";
std::cout << info.UnmapValue("True", 2) << "\n";
std::cout << info.UnmapValue("False", 2) << "\n";
Copy link
Member

Choose a reason for hiding this comment

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

Thanks, nice catch on this one. 👍

@AdarshSantoria
CFType to CF
2a3f3e6 
Change inorder to make CF typedef from CFType
@AdarshSantoria
typedef CF to CFType
50c797a
@AdarshSantoria
Copy link
Contributor Author

AdarshSantoria commented Feb 7, 2023 •
edited

One thing I want to ask src/mlpack/methods/src/cf/decomposition_policies/quic_svd_method.hpp is not there but is mentioned in documentation of cf so shall I add that file with reference to src/mlpack/methods/quic_svd.

@rcurtin
Copy link
Member

rcurtin commented Feb 8, 2023

One thing I want to ask src/mlpack/methods/src/cf/decomposition_policies/quic_svd_method.hpp is not there but is mentioned in documentation of cf so shall I add that file with reference to src/mlpack/methods/quic_svd.

Sure, if you see an easy way to hook it up, feel free! That would definitely be worth an entry in HISTORY.md, too.

@AdarshSantoria AdarshSantoria mentioned this pull request Feb 9, 2023
Merged
rcurtin
rcurtin reviewed Feb 9, 2023
View reviewed changes
Copy link
Member

@rcurtin rcurtin left a comment

Choose a reason for hiding this comment

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

Thanks, just one final comment. :)

doc/tutorials/cf.md Outdated
@@ -276,6 +276,9 @@ alternating least squares update rules). These include:
- `NMFALSFactorizer`
- `RegularizedSVD`
- `QUIC_SVD`
- `BiasSVD`
- `SVDPlusPlus`
- `RandomizedSVD`
Copy link
Member

Choose a reason for hiding this comment

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

I took a look and I think these names don't actually correspond to what's used in the code. So instead of RandomizedSVD, we should have RandomizedSVDPolicy, etc.; I think there are errors in what was already there before too.

rcurtin
rcurtin approved these changes Feb 9, 2023
View reviewed changes
Copy link
Member

@rcurtin rcurtin left a comment

Choose a reason for hiding this comment

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

Thanks again! 👍

@conradsnicta
Copy link
Contributor

suggest to also add a corresponding item to HISTORY.md

mlpack-bot[bot]
mlpack-bot bot approved these changes Feb 10, 2023
View reviewed changes
Copy link

@mlpack-bot mlpack-bot bot left a comment

Choose a reason for hiding this comment

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

Second approval provided automatically after 24 hours. 👍

rcurtin
rcurtin reviewed Feb 11, 2023
View reviewed changes
HISTORY.md Outdated Show resolved Hide resolved
@AdarshSantoria @rcurtin
Update HISTORY.md
e0c7866 
Co-authored-by: Ryan Curtin <ryan@ratml.org>
@rcurtin rcurtin merged commit fe23d1d into mlpack:master Feb 11, 2023
@rcurtin
Copy link
Member

rcurtin commented Feb 11, 2023

Thanks so much!

@rcurtin rcurtin mentioned this pull request Apr 27, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rcurtin rcurtin rcurtin approved these changes

@mlpack-bot mlpack-bot[bot] mlpack-bot approved these changes

Assignees
No one assigned
Labels
c: documentation t: bugfix
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@AdarshSantoria @rcurtin @conradsnicta