add queuepenalty #33
add queuepenalty #33
Conversation
timholy
left a comment
timholy
left a comment
There was a problem hiding this comment.
Very nice. You've done a good job of ensuring this change does not break the public API of the package, while being free to change unexported internals.
src/NMFMerge.jl
Outdated
| 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 |
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. |
timholy
left a comment
timholy
left a comment
There was a problem hiding this comment.
Still needs a test of nmfmerge(my_custom_queuepenalty, args...) (one where you show that different penalties result in different merge choices) before we can merge this.
timholy
left a comment
timholy
left a comment
There was a problem hiding this comment.
Let's merge and release a new version!
No description provided.