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

Improvement of two gallery examples regarding categorical colormaps #1934

Merged
merged 5 commits into from
Jun 9, 2022
Merged

Improvement of two gallery examples regarding categorical colormaps #1934

merged 5 commits into from
Jun 9, 2022

Conversation

yvonnefroehlich
Copy link
Member

Description of proposed changes

Fixes #1933

Reminders

  • Run make format and make check to make sure the code follows the style guide.
  • Add tests for new features or tests that would have caught the bug that you're fixing.
  • Add new public functions/methods/classes to doc/api/index.rst.
  • Write detailed docstrings for all functions/methods.
  • If wrapping a new module, open a 'Wrap new GMT module' issue and submit reasonably-sized PRs.
  • If adding new functionality, add an example to docstrings or tutorials.

Slash Commands

You can write slash commands (/command) in the first line of a comment to perform
specific operations. Supported slash commands are:

  • /format: automatically format and lint the code
  • /test-gmt-dev: run full tests on the latest GMT development version

@welcome
Copy link

welcome bot commented May 26, 2022

💖 Thanks for opening this pull request! 💖

Please make sure you read our contributing guidelines and abide by our code of conduct.

A few things to keep in mind:

  • If you need help writing tests, take a look at the existing ones for inspiration. If you don't know where to start, let us know and we'll walk you through it.
  • All new features should be documented. It helps to write the docstrings for your functions/classes before writing the code. This will help you think about your code design and results in better code.
  • No matter what, we are really grateful that you put in the effort to do this! 🎉

@yvonnefroehlich
Copy link
Member Author

I altered the code as suggested by @seisman in #1933 comment.

@yvonnefroehlich
Copy link
Member Author

yvonnefroehlich commented May 27, 2022

I am very sorry for accidentally closing this PR! I reopened it.

@seisman
Copy link
Member

seisman commented May 28, 2022

Ping @michaelgrund and @weiji14 for reviewing this PR, as they wrote the initial version of the example.

@seisman seisman requested review from michaelgrund and weiji14 May 28, 2022 16:28
@seisman seisman added the documentation Improvements or additions to documentation label May 28, 2022
@seisman seisman added this to the 0.7.0 milestone May 28, 2022
@yvonnefroehlich yvonnefroehlich changed the title Add remark regarding pandas categorical number code in two gallery examples Improvement of two gallery examples regarding categorical colormaps Jun 1, 2022
Copy link
Member

@weiji14 weiji14 left a comment

Choose a reason for hiding this comment

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

Thanks for taking up this Pull Request @yvonnefroehlich!

I've made a few suggested changes to the documentation wording, which you're welcome to "Commit suggestion" directly, or pool several together using "Add suggestion to batch" before committing. See also https://docs.github.com/en/github/collaborating-with-issues-and-pull-requests/incorporating-feedback-in-your-pull-request#applying-a-suggested-change.

image

# palette "cubhelix" in categorical format and add the species names as
# annotations for the colorbar
pygmt.makecpt(
cmap="cubhelix", color_model="+cSetosa,Versicolor,Virginica", series=(0, 2, 1)
cmap="cubhelix",
# Use the minum and maximum of the categorical number code
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
# Use the minum and maximum of the categorical number code
# Use the minimum and maximum of the categorical values (alphabetically sorted)

Copy link
Member Author

Choose a reason for hiding this comment

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

Pandas assigns numbers (in an alphabetical manner) to the individual categories, when converting df.species to the categorical data type:

df.species = df.species.astype(dtype="category")

These numbers are used to define the range of the colormap (series of pygmt.makecpt):

    series=(df.species.cat.codes.min(), df.species.cat.codes.max(), 1),

Thus, I think we should use here 'categorical number code'. Also because this term is later used in the comment for the color parameter of the pygmt.Figure.plot() or pygmt.Figure.3Dplot() method:

    # Points colored by categorical number code
    color=df.species.cat.codes.astype(int),

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 not that familiar with panda's categorical data type, but looking at the documentation and the functions calls df.species.cat.categories and df.species.cat.codes, I think categories and number code are better.

Copy link
Member

Choose a reason for hiding this comment

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

Hmm you're right, let's go with categories and number code then. Wasn't aware that pandas actually uses numbers for categorical data!

examples/gallery/3d_plots/scatter3d.py Outdated Show resolved Hide resolved
Comment on lines 20 to 24
# By default pandas assigns the categorical number code in an alphabetical
# manner to the individual categories. For a non-alphabetical order, you have
# to adjust the categorical number code. For handling and manipulating
# categorical data in pandas you may have a look at:
# https://pandas.pydata.org/docs/user_guide/categorical.html
Copy link
Member

@weiji14 weiji14 Jun 1, 2022

Choose a reason for hiding this comment

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

Suggested change
# By default pandas assigns the categorical number code in an alphabetical
# manner to the individual categories. For a non-alphabetical order, you have
# to adjust the categorical number code. For handling and manipulating
# categorical data in pandas you may have a look at:
# https://pandas.pydata.org/docs/user_guide/categorical.html
# By default, pandas sorts the individual categorical values in an alphabetical
# order. For a non-alphabetical order, you have to manually adjust the list of
# categorical values. For handling and manipulating categorical data in pandas,
# have a look at:
# https://pandas.pydata.org/docs/user_guide/categorical.html

Copy link
Member Author

Choose a reason for hiding this comment

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

There is a typo in the suggested change in line 21: you instead of ou.

Copy link
Member

Choose a reason for hiding this comment

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

Good spotting, I've corrected the suggested change.

pygmt.makecpt(cmap="inferno", series=(0, 2, 1), color_model="+cAdelie,Chinstrap,Gentoo")
pygmt.makecpt(
cmap="inferno",
# Use the minum and maximum of the categorical number code
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
# Use the minum and maximum of the categorical number code
# Use the minimum and maximum of the categorical values (alphabetically sorted)

@yvonnefroehlich
Copy link
Member Author

Thanks for taking up this Pull Request @yvonnefroehlich!

I have to say thank you for the nice and welcoming meeting by @meghanrjones and @weiji14 during the EGU conference! 😊

Also thanks to @seisman and @weiji14 for their reviews!
But I am a bit unsure how to continue. Should I submit a commit for fixing the typo and interpunction as well as shortening the text (corresponding to the review by @weiji14), but keeping ‘categorical number code’ and ‘categories’ (corresponding to the comment by @seisman)? Or will there be a new review by @weiji14? Or should we wait for the review by @michaelgrund?

@weiji14
Copy link
Member

weiji14 commented Jun 7, 2022

Also thanks to @seisman and @weiji14 for their reviews! But I am a bit unsure how to continue. Should I submit a commit for fixing the typo and interpunction as well as shortening the text (corresponding to the review by @weiji14), but keeping ‘categorical number code’ and ‘categories’ (corresponding to the comment by @seisman)? Or will there be a new review by @weiji14? Or should we wait for the review by @michaelgrund?

On second thought, let's keep using 'categories' or `number code' as suggested by @seisman in #1934 (comment) (sorry for the confusion, I was a bit mistaken with pandas categorical data). If @michaelgrund has time, it would be nice to get his input since he wrote this example originally, but if not, we can still continue with the Pull Request.

@yvonnefroehlich
Copy link
Member Author

On second thought, let's keep using 'categories' or `number code' as suggested by @seisman in #1934 (comment) (sorry for the confusion, I was a bit mistaken with pandas categorical data). If @michaelgrund has time, it would be nice to get his input since he wrote this example originally, but if not, we can still continue with the Pull Request.

No worries and no hurry @weiji14 😉
I am totally fine with waiting for the review by @michaelgrund before pushing a further commit.

@michaelgrund
Copy link
Member

I'm fine with all the changes made so far!

@yvonnefroehlich
Copy link
Member Author

I'm fine with all the changes made so far!

Thanks for your response @michaelgrund !

I updated the text based on the reviews and discussion.

Copy link
Member

@seisman seisman left a comment

Choose a reason for hiding this comment

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

Looks great!

@seisman seisman added the final review call This PR requires final review and approval from a second reviewer label Jun 9, 2022
Copy link
Member

@weiji14 weiji14 left a comment

Choose a reason for hiding this comment

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

Nice one, thanks again @yvonnefroehlich!

@weiji14 weiji14 merged commit 056483a into GenericMappingTools:main Jun 9, 2022
@welcome
Copy link

welcome bot commented Jun 9, 2022

🎉🎉🎉 Congrats on merging your first pull request and welcome to the team! 🎉🎉🎉

Please open a new pull request to add yourself to the AUTHORS.md file. We hope that this was a good experience for you. Let us know if there is any way that the contributing process could be improved.

@weiji14 weiji14 removed the final review call This PR requires final review and approval from a second reviewer label Jun 9, 2022
@yvonnefroehlich
Copy link
Member Author

Nice one, thanks again @yvonnefroehlich!

I have to say thank you for allowing and helping me to work on this issue / pull request! 😊

We hope that this was a good experience for you. Let us know if there is any way that the contributing process could be improved.

Definitely! 😊 - Likely I learned more than I brought to the project! 😉

@yvonnefroehlich yvonnefroehlich deleted the improve-examples-categorical-cpt branch June 10, 2022 20:41
sixy6e pushed a commit to sixy6e/pygmt that referenced this pull request Dec 21, 2022
…enericMappingTools#1934)

Clarify that by default the labels for the colorbar must be given in alphabetical order.

* add remark regarding categorical number code
* use flexible arguments in makecpt
* shorten and replace remark regarding categorical number code
* fix typos and shorten text based on review
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Suggestion for improvement of gallery examples regarding categorical colormaps
4 participants