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

Reorganize docs and add a quickstart guide #33

Merged
merged 22 commits into from
Jul 2, 2024

Conversation

keithchev
Copy link
Member

@keithchev keithchev commented Jul 1, 2024

The documentation for the package currently consists of a series of jupyter notebooks. This PR:

  • reorganizes these notebooks into a docs/ subdirectory.
  • adds a quickstart guide to provide a quick overview of how to use the package.
  • adds a documentation README with links to the notebooks, the quickstart guide, and the notebooks of example plots.

Note: github does not recognize that the style_usage.ipynb notebook was moved; it treats it as a new file, so there's no way to view the diff, which is annoying. (I was careful to move the file in one commit and modify it in a second commit, so the git history "knows" about the renaming; this must be a github thing).

Other changes

  • move the notebooks in /plot_testing into /docs/examples.
  • remove the PDF files of example plots that were previously generated by the style_usage notebook.
  • minor edits in the style_usage notebook for clarity/succinctness.
  • move ipykernel from a main dependency to a dev dependency (as it is only needed to run the documentation notebooks).

Style changes

These were made in discussion with Dennis.

  • increase the default title size (so that it is not smaller than the default legend title font size).
  • rename mpl.style_axis to style_plot for clarity.
  • rename the color lightgray to gray to be consistent with the style guide.
    -rename brown_shades to warm_gray_shades and grey_shades to cool_gray_shades.
  • renaming to use "gray" instead of "grey" everywhere.

PR checklist

  • Describe the changes you've made.
  • If you've added new software dependencies, make sure that those dependencies are included in the appropriate conda environments.
  • If you've added new functionality, make sure that the documentation is updated accordingly.

Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@keithchev keithchev force-pushed the kcc/reorg-docs-and-add-quickstart branch from 77bf00e to 38a8d83 Compare July 1, 2024 17:59
@keithchev keithchev marked this pull request as ready for review July 1, 2024 23:58
@keithchev keithchev requested a review from mezarque July 2, 2024 00:00
Copy link
Member

@mezarque mezarque left a comment

Choose a reason for hiding this comment

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

Looks mostly good to me! A couple of typos here and there to fix. I think it would also be useful to add images to the quickstart guide.

arcadia_pycolor/palettes.py Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

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

I'm wondering if we can add some images to this file, so that people can see view cell outputs; we could imagine having this be a Jupyter notebook that we convert to Markdown using jupyter nbconvert --to markdown quickstart.ipynb

Copy link
Member Author

@keithchev keithchev Jul 2, 2024

Choose a reason for hiding this comment

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

So this started out as a jupyter notebook and I ended up switching it to markdown because I felt that the outputs of most of the code snippets in the quickstart guide are not essential and may even be distracting, as the snippets are meant to illustrate how to use apc objects, not how to plot things. And of course markdown is much easier to write, review, and version control.

How do you feel about leaving this as markdown for now but adding static images to show the output of printing colors, palettes, and gradients? (which are code snippets whose outputs I feel it would definitely be helpful to show)

Copy link
Member

Choose a reason for hiding this comment

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

That seems reasonable. I think it would be useful to show the outputs of plt.show() calls as well, since I think those would convey the expected behaviors of apc.mpl.setup() and apc.mpl.style_plot().

Copy link
Member Author

@keithchev keithchev Jul 2, 2024

Choose a reason for hiding this comment

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

Added images for the color/palette/gradient swatches!

I guess I have a different feeling about adding images for the plt.show() outputs, which is that the quickstart guide should have a "just type this and your plots will be styled" vibe rather than "here is what your plots will look like". I think I have this feeling because:

  • it helps make the quickstart guide as succinct as possible.
  • the expected behaviors of setup and style_plot are already documented in thestyle_usage notebook and may be hard to grok without the before-and-after approach that that notebook uses (and that would feel too verbose in a quickstart guide).
  • if we show plot outputs, the quickstart guide will start to reproduce the style_usage notebook.

In lieu of showing plot outputs, how would you feel about adding more obvious/explicit links to the style_usage notebook in the quickstart guide?

Copy link
Member

Choose a reason for hiding this comment

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

I suppose it's fine not to show the plot examples in the quickstart then, as long as we link to the relevant before/after examples in style_usage!

Copy link
Member Author

Choose a reason for hiding this comment

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

Added very explicit links to style_usage to the quickstart. I think it's good enough for now; happy to revisit later!

docs/color_usage.ipynb Outdated Show resolved Hide resolved
docs/style_usage.ipynb Outdated Show resolved Hide resolved
@keithchev
Copy link
Member Author

@mezarque I made one minor change after you reviewed to add numpy as an explicit dependency. Since numpy was already a transitive dependency (of matplotlib), nothing was broken, but because some apc modules import numpy, it's good to include it among the direct deps.

Copy link
Member

@mezarque mezarque left a comment

Choose a reason for hiding this comment

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

Looks good to me for now!

@keithchev keithchev merged commit ef740c7 into feat/major-revamp Jul 2, 2024
1 check passed
@keithchev keithchev deleted the kcc/reorg-docs-and-add-quickstart branch July 2, 2024 21:20
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