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

Improving documentation (JOSS review) #41

Closed
jiwoncpark opened this issue Dec 16, 2020 · 14 comments
Closed

Improving documentation (JOSS review) #41

jiwoncpark opened this issue Dec 16, 2020 · 14 comments

Comments

@jiwoncpark
Copy link

These are some minor suggestions on improving the documentation in the README. openjournals/joss-reviews#2854

  • (optional) It'd be great if the README could point directly to the docs page, where the user can read the per-module docstrings. The "Generating Datasets" has the parameter description for make_dataset() in the markdown, but I wouldn't have known to look for it there.
  • "Getting Started" notebook: link for "Creating deeplenstronomy Configuration Files" is broken.
  • Currently, all the tests are in test_all.py and it's a bit hard to tell exactly what functionality of what module is being tested. A better organization would be to break it down into multiple files named after the modules being tested. Also, I strongly recommend setting up CI via GitHub Actions!
@rmorgan10
Copy link
Contributor

Links in the "Getting Started" Notebook are now fixed by commit 68ffd33

@shreyasbapat
Copy link

I won't create a new issue for the test suite since it is already mentioned here :D

@shreyasbapat
Copy link

I would also like to point out that even though the package has excellent docstrings pretty much everywhere, there's no simple way to see what the API looks like. I feel there is a need to have the API Documentation for the same. Which can be easily achieved using sphinx.

@rmorgan10
Copy link
Contributor

I've created a docs page to make the docstrings more accessible with commit a960561

https://deepskies.github.io/deeplenstronomy/docs

@rmorgan10
Copy link
Contributor

I've linked the docs page in the main readme so that users can easily find it with commit 7d1d0b7

@rmorgan10
Copy link
Contributor

@jiwoncpark I'm planning on addressing your comments about the tests in @shreyasbapat 's issue #42 . When you get a chance, could you look over the improvements I've made to the documentation and let me know if you feel like this issue can be closed?

@rmorgan10
Copy link
Contributor

@jiwoncpark I've also updated the paper text based on your recommendations with the following commits:

@pdebuyl
Copy link

pdebuyl commented Jan 20, 2021

The modules distributions and surveys don't have docstrings, it would be good to add them.

@rmorgan10
Copy link
Contributor

The modules distributions and surveys don't have docstrings, it would be good to add them.

@pdebuyl The reason I left docstrings out for those modules is they are internal functions, never called by the user. The only reason I included them in the documentation and didn't make them private methods was to provide a quickly accessible list of all possible surveys and distributions.

Given that information, do you think the docstrings are still necessary for those modules?

@pdebuyl
Copy link

pdebuyl commented Jan 25, 2021

Given that information, do you think the docstrings are still necessary for those modules?

Yes. But a minimal docstring is fine. Else, the only starting point to figure what a function does is its name or the source code, which is as good as not providing their listing.

@rmorgan10
Copy link
Contributor

@pdebuyl I've added docstrings to the distributions.py, surveys.py, and check.py modules with commit e906f49 and I've regenerated all the docs pages.

@rmorgan10
Copy link
Contributor

@jiwoncpark Can I go ahead and close this issue?

@jiwoncpark
Copy link
Author

LGTM! Yes, please go ahead.

@rmorgan10
Copy link
Contributor

Great! Thanks for the review!

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

No branches or pull requests

4 participants