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

add documentation #7

Merged
merged 3 commits into from
Jun 21, 2024
Merged

add documentation #7

merged 3 commits into from
Jun 21, 2024

Conversation

youdongguo
Copy link
Member

No description provided.

Copy link

codecov bot commented Jun 3, 2024

Codecov Report

Attention: Patch coverage is 81.63265% with 18 lines in your changes missing coverage. Please review.

Project coverage is 81.63%. Comparing base (5321ac4) to head (7bfe8de).

Files Patch % Lines
src/NMFMerge.jl 81.63% 18 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main       #7   +/-   ##
=======================================
  Coverage   81.63%   81.63%           
=======================================
  Files           1        1           
  Lines          98       98           
=======================================
  Hits           80       80           
  Misses         18       18           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@timholy
Copy link
Member

timholy commented Jun 6, 2024

Typically one would split this into two pull requests:

  • one PR fixes the bug, and introduces a test case that would have caught the bug had that test been present
  • the other PR adds the documentation

To add a little more detail about the first one: once you've identified a bug, the following workflow is great:

  • check out the main branch (the one with the bug)
  • based on main, create a new branch that will contain your fix
  • add a test case to the test suite that fails when you run Pkg.test()
  • make a commit (so now your new branch has a test that reveals the bug, but it does not yet include the fix)
  • make the fix to the source code, check that the tests now pass
  • commit the fix
  • submit the PR
  • you can squash the two commits when you merge the PR

There are many nice things about this workflow:

  • if someone else "owns" the package, they can see the bug themselves by checking out a branch that includes the first commit but not the second. This can be crucial for convincing someone that you've identified a genuine problem.
  • it's very easy to think you've written a test that would have caught the bug, only to discover later that for unanticipated reasons it doesn't. For being sure your test suite is really exercising the functionality you need to be able to rely on, there's no substitute for verifying that the old code fails. Once you've done that, you know you're on your way to having a robust test suite that will catch any bugs that you or others might introduce later.

@timholy
Copy link
Member

timholy commented Jun 18, 2024

Not sure if this is ready for review or if you're still working on this, but a couple of comments. First, this is definitely moving in the right direction. TODO:

  • remove the bugfix (that's in fix code bug #8 now)
  • I'd generally start with the runnable example and then give details about the functions later. The general idea is that reading specifics about a function is a bit tedious, and if you don't yet know if you even want to use that function then you're not sure why you should care. If you do the demo first, people are motivated to learn more.
  • Consider annotating the steps in the demo of your code. E.g., "Run over-complete NMF (5 components instead of 4):" followed by the corresponding line(s) of Julia code. Also, do you need so many kwargs for the NMF? It makes it look finicky. You may also want to emphasize that you're running a lot of iterations of standard NMF and it's still not finding a good minimum.
  • The README makes me think we need an overall convenience wrapper, e.g., nmfmerge(X, 5 => 4). You could show that first and then walk people through the steps that it carries out, so they can see the consequences for themselves.
  • Add docstrings to the functions (consider removing some of that from the README, or display the result of ?colmerge2to1pq)
  • If you don't think this package will ever use Documenter documentation, then remove the badges from the README and comment out the whole docs: job from .github/workflows/CI.yml. Otherwise, if a user clicks on those links hoping to be taken to the documentation, they'll get a 404 error which does not seem very polished.

Copy link
Member

@timholy timholy left a comment

Choose a reason for hiding this comment

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

Small suggestions on the docstrings.

src/NMFMerge.jl Outdated
"""
colnormalize(W, H, p)

This function normalize ||W[:, i]||_p = 1 for i in 1:size(W, 2)
Copy link
Member

Choose a reason for hiding this comment

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

You might mention that any change in scaling is transferred to the corresponding H component.

src/NMFMerge.jl Outdated
@@ -16,8 +16,25 @@ function colnormalize!(W, H, p::Integer=2)
end
return W, H
end

"""
colnormalize(W, H, p)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
colnormalize(W, H, p)
colnormalize(W, H, p=2)

To show the default value

src/NMFMerge.jl Outdated Show resolved Hide resolved
@youdongguo
Copy link
Member Author

wrong close

@youdongguo youdongguo reopened this Jun 18, 2024
@youdongguo youdongguo changed the title fix \xi and u (wrong in old version), add read me add documentation Jun 19, 2024
@youdongguo youdongguo merged commit 8b148ab into main Jun 21, 2024
2 checks passed
@youdongguo youdongguo deleted the yd/add_readme branch June 21, 2024 21:49
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.

2 participants