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

Make docs consistent re: default/min parameters #41

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

fedarko
Copy link
Contributor

@fedarko fedarko commented Jul 10, 2019

I think there might still need to be some changes made regarding parameter names (since it looks like --rank has been changed to --n_components, and --iterations has been changed to --max_iterations), but I'm not sure what to do there.

this is a low priority fix so no worries :)

I'd commented out the ground-truth checking code (due to the
randomness of the test inputs), so the docstring that said the test
checked against ground truths ended up being inaccurate. (My b)
It looked like there were a few inconsistencies. Nothing major
(if the user passed in something with --rank 1 / --n_components 1
then they should just get an error) but probably worth addressing
at some time
Copy link
Collaborator

@cameronmartino cameronmartino left a comment

Choose a reason for hiding this comment

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

Good catch. Thanks, @fedarko!

@fedarko
Copy link
Contributor Author

fedarko commented Aug 3, 2019

Added another commit that shills Qurro's moving pictures tutorial, now that that's ready :P

@cameronmartino
Copy link
Collaborator

@fedarko this looks okay to merge. 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.

2 participants