add queuepenalty #33
add queuepenalty #33
Conversation
src/NMFMerge.jl
Outdated
| @@ -221,4 +220,7 @@ function mergecolumns(W::AbstractArray, H::AbstractArray, mergeseq::AbstractArra | |||
| return Smtx, Matrix(Tmtx), STstage, Err | |||
| end | |||
|
|
|||
| mergepenalty(λ_min, t1sq, t2sq) = λ_min | |||
| shotpenalty(λ_min, t1sq, t2sq) = λ_min / sqrt(min(t1sq, t2sq)) | |||
There was a problem hiding this comment.
Should we consider exporting mergepenalty and shotpenalty? If so, they would need docstrings and each to have tests. Alternatively we could keep them private. If so, then since shotpenalty is untested perhaps it should not be in this package?
There was a problem hiding this comment.
Deleted shotpenalty, I think keeping it private at this moment is better as I am not sure how to describe the queuepenalty in a good way. We have a queuepenalty work in a fixed way as: f(E, t1sq, t2sq), users cannot define a customized penalty function.
There was a problem hiding this comment.
Once this merges, they will be able to define their own queuepenalty, but I like the idea of not exporting anything.
|
Either in this PR or after you merge it, you should release this as v1.1.0 of the package. (It's a new feature, so it should bump the minor version number.) One more thought, it might be worth mentioning the possibility of passing in a |
|
|
||
| Performs "NMF-Merge" on data matrix `X`. | ||
|
|
||
| Arguments: | ||
|
|
||
| -`queuepenalty`: a function of the form `f(E, t1sq, t2sq)` that computes the penalty for merging two components, where `E` is the the merge error described in the paper, default: f(E, t1sq, t2sq)=E. | ||
|
|
There was a problem hiding this comment.
I added yesterday but forgot to mention in the conversation. I am confused about how to describe it in a good way.
There was a problem hiding this comment.
Maybe put the queuepenalty argument in square brackets, as that often indicates, an optional argument: result = nmfmerge([queuepenalty,] X, ncomponents; ...). An alternative is to use multiple lines:
result = nmfmerge(X, ncomponents; ...)
result = nmfmerge(queuepenalty, X, ncomponents; ...)
Performs "NMF-Merge" on data matrix `X`. ...In the API for queuepenalty you should also describe what t1sq and t2sq are.
There was a problem hiding this comment.
Just a reminder that only a few things (mostly documentation) remain before merging:
- indicate that
queuepenaltyis an optional argument tonmfmerge, e.g., withnmfmerge([queuepenalty,] X, ...)or by using two lines, one supplyingqueuepenaltyand one not (see #33 (comment)) - describe the meaning of the
t1sqandt2sqarguments forqueuepenalty - Docstring for
colmerge2to1pq(which is also exported) should be updated to includequeuepenalty. You can refer readers to the docstring fornmfmergefor an explanation about thequeuepenaltyAPI, rather than repeating the same material all over again. - It would be ideal to add a test where the user supplies a custom
queuepenaltyand verify that it changes the merge order - bump the version number to 1.1.0 in the Project.toml so this is ready to register as a new version
|
Bump |
|
Still needs a test. You could probably use something like nmfmerge(X, ...) do E, t1sq, t2sq
return -E
endand then test that it picks out the worst pair to merge. |
No description provided.