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

Added the coordinate descent method from scikit-learn, supporting sparsity #23

Merged
merged 13 commits into from
Jul 24, 2019

Conversation

vilim
Copy link
Contributor

@vilim vilim commented Mar 2, 2018

This is a more-or-less direct translation of the Python Scikit-learn NMF method, which hopefully respects Julian idioms.

I am not sure how I should reference the original implementation, so any comments on attribution, licences and copyright are welcome.

@kmsquire
Copy link

kmsquire commented Mar 3, 2018

Here's an example of some publicly available code that was translated from Matlab and placed in Images.jl.

Scikit-learn code is available under a BSD-style license. This package is MIT-licensed. Generally, these licenses are compatible with each other, so as long as the core maintainers of this package are open to it, a typical solution would be to note an exception in LICENSE.md, which states that the package is available under the MIT license (already there), except as noted at the top of individual files, and then to include the copyright and BSD license text at the top of coorddesc.jl.

As long as you include the copyright and license somewhere, any variation on above should be fine.

@ararslan
Copy link
Member

except as noted at the top of individual files

Note that BSD style licenses are not copyleft, so we don't need to have an exception in our license for code derived from BSD-licensed code; BSD style licenses permit licensing derived works under any license. We can simply acknowledge and link to the license for the code from which this was derived directly in the file.

@vilim
Copy link
Contributor Author

vilim commented Feb 11, 2019

@ararslan I have already updated the file referencing the original implementation and the license, is there anything else to do before it can be merged? The same Travis test error is also present on the master branch.

Project.toml Outdated Show resolved Hide resolved
Project.toml Show resolved Hide resolved
Manifest.toml Outdated Show resolved Hide resolved
src/coorddesc.jl Outdated Show resolved Hide resolved
src/coorddesc.jl Outdated Show resolved Hide resolved
src/coorddesc.jl Outdated Show resolved Hide resolved
@ararslan
Copy link
Member

I have already updated the file referencing the original implementation and the license

I saw, that looks good. My comment about the license was in response to Kevin's.

@vilim
Copy link
Contributor Author

vilim commented Feb 12, 2019

@ararslan Thanks for the review! I have implemented all the changes.

@ararslan
Copy link
Member

Looks like the Project.toml changes weren't implemented.

@vilim
Copy link
Contributor Author

vilim commented Feb 13, 2019

@ararslan sorry, I switched to VS Code recently and am still getting used to file status indicators; it wasn't committed. It is fixed now.

@vilim
Copy link
Contributor Author

vilim commented Feb 15, 2019

@ararslan is there anything else I need to do before this can be merged?

@VPetukhov
Copy link
Contributor

VPetukhov commented Jul 24, 2019

Hi @ararslan ! Can I ask what's blocking from merging this PR? Looks like the requested changes were resolved. And I can't reproduce the test failure: now it works absolutely fine. Here is travis the successful report from my fork (without any changes): https://travis-ci.com/VPetukhov/NMF.jl/builds/120496869 . So can you please maybe trigger rebuild and check if it still fails?

@ararslan ararslan closed this Jul 24, 2019
@ararslan ararslan reopened this Jul 24, 2019
@ararslan ararslan merged commit affbab8 into JuliaStats:master Jul 24, 2019
@VPetukhov
Copy link
Contributor

Thank you!

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

Successfully merging this pull request may close these issues.

None yet

4 participants