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.

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

NMF: allow user to specify one of initial W/H matrices #1367

Closed
wants to merge 6 commits into from

Conversation

wenhaoh2
Copy link
Member

This PR is follow-up of the discussion in #1261. It adds support to allow user to specify one of initial H/W matrices, and to randomly initialize the other one.

@zoq
Copy link
Member

zoq commented Apr 17, 2018

This is an interesting idea, but I'm not sure this is the best way to do it, e.g., in this case, a user could call randu or randn on the matrix an pass it with the existing GivenInitialization rule. Perhaps, a combination is something that could be explored, by combination I mean, a user could merge two rules. Let me know what you think.

@wenhaoh2
Copy link
Member Author

@zoq Thanks for the suggestion:) Actually there is a small difference between this implementation and what you meant. By this implementation, one of W/H matrix will be initialized randomly each time Initialize() is called. So if the same GivenInitialization instance is used multiple times, the randomly initialized matrices are different.
And to ensure I correctly understand, by combination of rules, do you mean that for example, W is initialized with existing GivenInitialization and H is initialized with existing RandomInitialization?

@zoq
Copy link
Member

zoq commented Apr 18, 2018

You are right, I thought about GivenInitialization as a one time initialization rule.

About the merge idea, you are right, my only concern is, right now each rule will initialize W and H, maybe the performance in this case is negligible.

@wenhaoh2
Copy link
Member Author

@zoq that's a good idea. What I have in mind is to use a class which takes two Initialization Rules as template parameters, and initialize W with the first and H with the second.

template<typename WInitializationRuleType, typename HInitializationRuleType>
class MergeInitializaition
{
  // ...
};

I think the performance overhead is negligible, compared to the time used to compute the iterations.
But my only concern is that this might not be a common usage.

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.

Hi Wenhao,

Thanks for your hard work with this. I think that the MergeInitialization idea is fine if you want to refactor to that.

The main programs don't current call Initialize() more than once, so there is a copy of W and H that happens, but I think generally these will be much smaller than the data V (assuming rank is reasonably small, which is probably a good assumption), so the cost of the copy should be pretty negligible. I can't think of an easy way to avoid it.

* multiple AMF objects, but will incur copies of the W and H matrices.
* This initialization rule for AMF simply fills the W and/or H matrices with
* the matrices given to the constructor of this object. If only one initial
* matrix is given, the other matrix will be initialized with random noise.
Copy link
Member

Choose a reason for hiding this comment

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

This is a little picky but for clarity we should probably write uniform random noise instead of random noise throughout.

@wenhaoh2
Copy link
Member Author

I'm sorry for the late reply because I have been writing my bachelor thesis. I will refactor the code to the MergeInitialization idea once I finish the thesis, which may take a week or so:)

@zoq
Copy link
Member

zoq commented Apr 30, 2018

Sounds good, hopefully you have some fun to finish the last part of your thesis.

@mlpack-bot
Copy link

mlpack-bot bot commented Jul 19, 2019

This issue has been automatically marked as stale because it has not had any recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions! 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants