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

[ENH]: data kwarg support for mplot3d #20912 #20951

Merged
merged 8 commits into from Sep 19, 2021

Conversation

jyjoshi
Copy link
Contributor

@jyjoshi jyjoshi commented Aug 31, 2021

PR Summary

Fix #20912

Hi, I have started working on this issue. This will be my first open source contribution, so I will need some help and guidance. I have gone through the contributing guide and have setup the development environment and Matplotlib in editable mode on my system.

I am currently working on understanding the Matplotlib internals.

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Thank you for opening your first PR into Matplotlib!

If you have not heard from us in a while, please feel free to ping @matplotlib/developers or anyone who has commented on the PR. Most of our reviewers are volunteers and sometimes things fall through the cracks.

You can also join us on gitter for real-time discussion.

For details on testing, writing docs, and our review process, please see the developer guide

We strive to be a welcoming and open project. Please follow our Code of Conduct.

@story645
Copy link
Member

story645 commented Sep 2, 2021

Hi, thanks for the commit but you accidentally included a whole bunch of files. Please remedy that by

  1. stash any current changes https://git-scm.com/docs/git-stash
  2. checkout master
  3. create a new branch off master
  4. add your changes to the new branch
  5. just add and commit the files you changed
  6. push.

It's fine if you open a new PR to do this as I figure starting over with a new branch will be easier than cleaning up this one.
If you need help, please ask on gitter, especially the incubator channel https://matplotlib.org/devdocs/devel/contributing.html#contributor-incubator

@jyjoshi
Copy link
Contributor Author

jyjoshi commented Sep 2, 2021

Hi, I noticed that I might have added my virtual environment files to the commit, in a hurry. I will try to cleanup this branch first and in case that does not work out, I will create a new PR and close this one.
Thanks :)

@timhoffm
Copy link
Member

timhoffm commented Sep 2, 2021

You can force-push over this PR.

  1. git checkout 20912-data-kwarg-support-mplot3d
  2. cleanup your branch
  3. git push origin 20912-data-kwarg-support-mplot3d --force

There are multiple ways to cleanup a branch, with more or less git magic.
If that's too complicated, a semi-manual way ist:

2.1) Backup the changes you still want.
2.2) git reset --hard master
2.3) Apply your changes again.

Anyway, the important message is: you can git push ... --force a branch for a PR to replace everything in there.

@jyjoshi jyjoshi marked this pull request as ready for review September 11, 2021 12:26
@jyjoshi
Copy link
Contributor Author

jyjoshi commented Sep 12, 2021

Hi, I have added the @_preprocess_data decorator to the scatter method in mplot3d/axes3d.py and it seems to work properly.

Code:

import matplotlib.pyplot as plt
>>> plt.gcf().add_subplot(projection="3d").scatter("a", "b", "c", data={"a": [0], "b": [1], "c": [2]})
<mpl_toolkits.mplot3d.art3d.Path3DCollection object at 0x7f80872b9400>
>>> plt.show()

Figure_1

It gives the desired output.

I have a couple of queries.

  1. What labels should be included by default in replace names and label_namer.
  2. How to resolve the checks that are currently failing.

@timhoffm
Copy link
Member

How to resolve the checks that are currently failing.

Place this before the **kwargs parameter entry in the docstring.

        data : indexable object, optional
            DATA_PARAMETER_PLACEHOLDER

We auto-generate the content for the description.

@jyjoshi
Copy link
Contributor Author

jyjoshi commented Sep 16, 2021

Hi,
I have added the @_preprocess_data decorator to relevant methods in mplot3d.py. All the tests are passing except Codecov/project/tests. Is it safe to merge?

@story645
Copy link
Member

Is it ok that this isn't tested?

@timhoffm
Copy link
Member

I think that's okish not to have tests. preprocess_data() itself is tested extensively. So tests would only add integration testing between the decorator and the 3d plotting functions. We also don't explicitly test combininations of positional and keyword arguments.

Systematically adding new tests for data kwarg an all functions seems overkill. Maybe one could add one or two image comparison tests and/or change some existing tests to use data. But given this is a first time contribution, I want to keep the barrier low.

@story645 story645 merged commit b8dcf0b into matplotlib:master Sep 19, 2021
@story645
Copy link
Member

Congrats on your first pr @jayjoshi112711! 🥳
We hope to see you around!

@jyjoshi
Copy link
Contributor Author

jyjoshi commented Sep 21, 2021

Thanks, @story645 and @timhoffm for helping me with my first PR, looking forward to making more contributions :)
Cheers!

@QuLogic
Copy link
Member

QuLogic commented Sep 25, 2021

@meeseeksdev backport to v3.5.x

@QuLogic QuLogic added this to the v3.5.0 milestone Sep 25, 2021
meeseeksmachine pushed a commit to meeseeksmachine/matplotlib that referenced this pull request Sep 25, 2021
timhoffm added a commit that referenced this pull request Sep 25, 2021
…951-on-v3.5.x

Backport PR #20951 on branch v3.5.x ([ENH]: data kwarg support for mplot3d #20912)
tacaswell pushed a commit to tacaswell/matplotlib that referenced this pull request Oct 12, 2021
…arg-support-mplot3d

[ENH]: data kwarg support for mplot3d matplotlib#20912
tacaswell pushed a commit that referenced this pull request Oct 20, 2021
…t-mplot3d

[ENH]: data kwarg support for mplot3d #20912
ericpre pushed a commit to ericpre/matplotlib that referenced this pull request Oct 20, 2021
…arg-support-mplot3d

[ENH]: data kwarg support for mplot3d matplotlib#20912
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.

[ENH]: data kwarg support for mplot3d
5 participants