Skip to content

Conversation

@jeromefv
Copy link
Contributor

@jeromefv jeromefv commented Oct 26, 2020

PR Summary

PR Checklist

  • Has pytest style unit tests (and pytest passes).
  • Is Flake 8 compliant (run flake8 on changed files to check).
  • New features are documented, with examples if plot related.
  • Documentation is sphinx and numpydoc compliant (the docs should build without error).
  • Conforms to Matplotlib style conventions (install flake8-docstrings and run flake8 --docstring-convention=all).
  • New features have an entry in doc/users/next_whats_new/ (follow instructions in README.rst there).
  • API changes documented in doc/api/next_api_changes/ (follow instructions in README.rst there).

# input. Classes that are 'array-like' such as `pandas` data objects
# and `numpy.matrix` may or may not work as intended. It is best to
# input. Classes that are similar to arrays such as `pandas` data objects
# and `numpy.matrix` may not work as intended. Common convention is to
Copy link
Member

Choose a reason for hiding this comment

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

You can remove numpy.matrix. That class is not recommended anymore by numpy itself, and rarely used in practice nowadays. - Also the matrix example code below.

Copy link
Member

Choose a reason for hiding this comment

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

I think this is a bit too strong, we do test that pandas works correctly in some cases and special case extracting the index and label when it makes sense.

Copy link
Member

@story645 story645 left a comment

Choose a reason for hiding this comment

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

mostly small things/nits but I like the changes here.

# can be defined e.g. in the :file:`matplotlibrc` file (see
# :doc:`/tutorials/introductory/customizing` for more information about
# the :file:`matplotlibrc` file). :rc:`path.simplify` is a boolean
# the :file:`matplotlibrc` file). :rc:`path.simplify` is a Boolean
Copy link
Member

Choose a reason for hiding this comment

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

I think boolean is lowercase?

Copy link
Contributor Author

@jeromefv jeromefv Apr 6, 2021

Choose a reason for hiding this comment

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

Found some inconsistencies in the Python docs, Built-in Types use the capital "B" and Boolean Objects does not.

Copy link
Member

@tacaswell tacaswell left a comment

Choose a reason for hiding this comment

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

I am 👍 on these changes. I agree with many of Tim's suggestions, but in the spirit of "does this makes the docs better" I would be fine with this going in either way.

@tacaswell
Copy link
Member

@jeromefv Ping me if you want a hand with the rebase.

@dstansby
Copy link
Member

dstansby commented Apr 5, 2021

👋 @jeromefv are you still planning on finishing this? I think it just needs a rebase. If not, I'm happy to finish it off instead.

@jeromefv
Copy link
Contributor Author

jeromefv commented Apr 5, 2021

Thank you, @dstansby! I'll work on it today and rebase with the changes. I'll ping any folks if I run into issues. Thank you for the update!

@jeromefv
Copy link
Contributor Author

jeromefv commented Apr 6, 2021

Overall, a revamp of this document would fit more in line with how I'd like to create consistency between this and the Getting Started guide in my other PR. I think it would take quite a bit of time to approach this again with much more in-depth rewriting.

@story645
Copy link
Member

story645 commented Apr 6, 2021

@jeromefv just to clarify, does that mean you'll finish out this PR but think more or needs to be done or should @dstansby finish it out?

@jeromefv
Copy link
Contributor Author

jeromefv commented Apr 6, 2021

@jeromefv just to clarify, does that mean you'll finish out this PR but think more or needs to be done or should @dstansby finish it out?

That's a good point! If @dstansby will/wants to take over and do bigger sweeping changes, then it makes a lot of sense for me to sit out here. Originally, this PR was more of a "practice" for me to get an idea of how I can shape the language to better fit the explanations.

I think if I were to tackle this PR again, I'd do a large restructuring project with explanations similar to what GSoD was. I don't think I'd be able to do that in a prompt timeline at the moment, however.

@jeromefv jeromefv force-pushed the usage_guide_edit branch from 29d35d4 to 951b2ff Compare May 4, 2021 03:35
@jklymak jklymak requested a review from story645 May 9, 2021 00:44
@jklymak
Copy link
Member

jklymak commented May 9, 2021

@story645 you were blocking on this.

@jklymak jklymak merged commit 3efd447 into matplotlib:master May 9, 2021
@QuLogic QuLogic added this to the v3.5.0 milestone May 10, 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.

7 participants