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

Docs: Initial Instructions for Notebook Leads #131

Merged
merged 8 commits into from
Feb 12, 2021

Conversation

robelgeda
Copy link
Collaborator

@robelgeda robelgeda commented Jan 19, 2021

This PR is the second stage of #127.

This PR migrates information from the sources listed in #128. We intend on removing duplicated docs (original sources) that are no longer needed.

Rendered Doc (To be removed once this PR is merged):
https://robel-dat-pyinthesky.readthedocs.io/en/docs/

Post-Merge: Rendered Doc of my branch has been disabled

Copy link
Contributor

@ibusko ibusko left a comment

Choose a reason for hiding this comment

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

This looks really good.

@camipacifici
Copy link
Contributor

This looks great!!

A few comments:

  • I would not list Imviz, until it sees the light
  • In "JDAT Notebooks": "These notebooks will eventually be made available...", but the plan is that they are publicly available right away, so maybe "These notebooks will be available..."; About the Note, I would either remove the detail about making a PR in the dat_pyinthesky repository, or add a link to the page where this is explained.
  • The instructions on how to install an environment from the requirement file deserve their own page, I think. This will be the most important page for people who comes to readthedocs to learn how to use the existing notebooks. The page can contain again the links to how to set up conda, then basic instructions on how to create an empty environment and how to add packages using the environment file. A reminder to contact the help desk in case of problems would also be useful.
  • In "STScI Notebook Leads", "Notebook Draft": there is line saying that instructions on how to fork and clone are at the end of the page, but the instructions are actually in another page.
  • In "STScI Notebook Leads", "Join a sprint": the notebook presentation and sprint review have been moved to Monday.

Great job!!

Base automatically changed from master to main February 4, 2021 22:46
Copy link
Collaborator

@eteq eteq left a comment

Choose a reason for hiding this comment

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

I haven't given this a thorough read-through yet, but my "skim" level review is that this is great! A few quick suggestions, though:

  • I think the style guides need to be linked more prominently in the main discussion. PArticularly https://github.com/spacetelescope/style-guides/blob/master/guides/where-to-put-your-data.md in the data section and https://github.com/spacetelescope/style-guides/blob/master/guides/jupyter-notebooks.md in probably a few different places (check list, notebook section, etc). I know they're in the "useful links", but I think we need to promote those in particular from "might read for background" to "must read and really should follow unless you have a thought out reason not to!".
  • The "data files" section currently assumes the person reading it is an ST staff member. Right now that's mostly OK, but maybe there should be a clarification that says something like "if you aren't an ST staff member, ask one of the JDAT team members to do this for you when you submit your PR"? In principal anyone can submit one of these notebooks, and while this hasn't happened yet it's worth preparing for the possibility now.
  • I wonder if we should have this PR be in jdat_notebooks instead of dat_pyinthesky? I'm thinking that because this repo has a bunch of other random "sandbox"-style stuff, whereas jdat_notebooks is specifically for notebooks. Then in dat_pyinthesky we can put in the readme "for docs for the notebook stuff see "?

@robelgeda
Copy link
Collaborator Author

Thanks for the review @eteq:

Sounds good to me, I moved them up to the top of the notebook and data pages.

  • The "data files" section currently assumes the person reading it is an ST staff member. Right now that's mostly OK, but maybe there should be a clarification that says something like "if you aren't an ST staff member, ask one of the JDAT team members to do this for you when you submit your PR"? In principal anyone can submit one of these notebooks, and while this hasn't happened yet it's worth preparing for the possibility now.

Makes sense, I added this in the important box in the data section.

  • I wonder if we should have this PR be in jdat_notebooks instead of dat_pyinthesky? I'm thinking that because this repo has a bunch of other random "sandbox"-style stuff, whereas jdat_notebooks is specifically for notebooks. Then in dat_pyinthesky we can put in the readme "for docs for the notebook stuff see "?

This is a good point and @camipacifici also mentions this in her review though I think we should add another doc for users. These docs should only be for notebook leads to avoid confusion. One of the most common mistakes in leads and the science reviewers is not knowing which repo is which. Having the leads go to jdat_notebooks for docs so they can contribute dat_pyinthesky might be confusing. In the same note, having users exposed to the detailed submission guidelines might not be useful (I noticed users want quick answers). For this reason, I think the best way to go might be leaving the submission docs here and updating the readme for notebook users in jdat_notebooks. If we need in-depth info for jdat_notebooks, I think the two types of docs are different enough to warrant a separate set of docs. This way the user never has to worry about the details of dat_pyinthesky and the notebook leads never have to go to jdat_notebooks during their dev process. Of course, we will advertise contributing in jdat_notebooks and provide a link to the docs in this PR! How does this sound?

Copy link
Collaborator

@duytnguyendtn duytnguyendtn left a comment

Choose a reason for hiding this comment

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

Thanks so much @robelgeda! There's so much more info in here than I was expecting!

One small sphinx build weirdness. After installing via the rtd-pip-requirements file, I tried to build the docs folder with:

cd docs
sphinx-build -b html . ..\build

which spat a Theme error:

Running Sphinx v3.4.3

Theme error:
no theme named 'sphinx_rtd_theme' found, inherited by 'stsci_rtd_theme

I ended up fixing it by uninstalling the pypi version of stsci_rtd_theme and reinstalling directly from github via:
pip install git+https://github.com/spacetelescope/stsci_rtd_theme.git

I don't think this is anything on us, but I'm leaving this breadcrumb just in case it helps someone else in the future

@duytnguyendtn
Copy link
Collaborator

Looks like @robelgeda has addressed all of @eteq and @camipacifici's concerns. With two approvals, this meets our acceptance criteria, so I'm going to go ahead and merge this PR. Thanks @robelgeda!

@duytnguyendtn duytnguyendtn merged commit 0e9e914 into spacetelescope:main Feb 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants