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

Create box3d example #19363

Merged
merged 20 commits into from
May 3, 2021
Merged

Create box3d example #19363

merged 20 commits into from
May 3, 2021

Conversation

iuryt
Copy link
Contributor

@iuryt iuryt commented Jan 26, 2021

Suggestion from #19310

PR Summary

This pull request adds a new example for the 3D plots.
I am not sure how to check all steps from the Checklist, but it is simply adding a new example.

Origin of this suggestion: #19310

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).

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.

Copy link
Contributor Author

@iuryt iuryt left a comment

Choose a reason for hiding this comment

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

I have implemented all suggested changes.

  • Missing whitespaces
  • Replace single by double quotes

Copy link
Contributor Author

@iuryt iuryt left a comment

Choose a reason for hiding this comment

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

I have incorporated the latest changes.

Copy link
Contributor Author

@iuryt iuryt left a comment

Choose a reason for hiding this comment

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

Incorporated pflakes suggestions

Copy link
Contributor Author

@iuryt iuryt left a comment

Choose a reason for hiding this comment

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

Incorporate pflakes suggestions

Copy link
Contributor Author

@iuryt iuryt left a comment

Choose a reason for hiding this comment

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

Incorporate pflakes suggestions

Copy link
Contributor Author

@iuryt iuryt left a comment

Choose a reason for hiding this comment

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

Incorporated PyLint suggestions.

@iuryt
Copy link
Contributor Author

iuryt commented Jan 26, 2021

Hi @jklymak
This is my first pull request to matplotlib.
I don't know what circleci stands for.
What should I do?

@jklymak
Copy link
Member

jklymak commented Jan 26, 2021

Circle CI builds the docs for you. If it is failing. you likely have a syntax error or a reference that doesn't resolve. Have a look at the log output (click on Details), and do a case-sensitive search on WARNING usually will help you find it.

@QuLogic
Copy link
Member

QuLogic commented Jan 26, 2021

We do not have xarray installed for docs builds, and I'm not sure we want to. Can this example be written without it?

@jklymak
Copy link
Member

jklymak commented Jan 26, 2021

Yeah, definitely no xarray (as much as I love xarray)

@iuryt
Copy link
Contributor Author

iuryt commented Jan 27, 2021

Ok! I will rewrite it without xarray.

Copy link
Contributor Author

@iuryt iuryt left a comment

Choose a reason for hiding this comment

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

Replaced Xarray by NumPy

Copy link
Contributor Author

@iuryt iuryt left a comment

Choose a reason for hiding this comment

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

Incorporate flake8 suggestions

Copy link
Contributor Author

@iuryt iuryt left a comment

Choose a reason for hiding this comment

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

Breaking long lines.

@iuryt
Copy link
Contributor Author

iuryt commented Jan 27, 2021

Hi @QuLogic and @jklymak ,

I have updated the script using NumPy instead of Xarray and it still fails on CircleCI.

Copy link
Member

@jklymak jklymak left a comment

Choose a reason for hiding this comment

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

Great! Now you have a bunch of flake8 errors though that need to be fixed. Sorry, they are annoying, but help keep the code semi-consistent....

@iuryt
Copy link
Contributor Author

iuryt commented Jan 27, 2021

@jklymak
Im sorry, but here it is saying that the flake8 test was successful.
Does it do another flake8 test inside circleci?

@jklymak
Copy link
Member

jklymak commented Jan 27, 2021

oops, it seems fine now. Thanks.

@QuLogic
Copy link
Member

QuLogic commented Jan 28, 2021

The example is still failing to run... Does it work for you?

@iuryt
Copy link
Contributor Author

iuryt commented Jan 28, 2021

If I run here, this is the output:

image

Matplotlib version: '3.2.2'
NumPy version: '1.16.4'

@jklymak
Copy link
Member

jklymak commented Jan 28, 2021

Hmmm, well unfortunately it has to pass on CI. Click through the links under "Checks" and you will see how this is failing.

@QuLogic
Copy link
Member

QuLogic commented Jan 29, 2021

The failure appears to be a regression. It is partially from eb76378, which causes the YZ plane to break, and partially from 4428da6, which breaks it completely.

Unfortunately, 3.3.4 was just tagged, so it won't be fixed there, but I think we should fix this for 3.4.0.

@QuLogic QuLogic added this to the v3.4.0 milestone Jan 29, 2021
@QuLogic QuLogic changed the title Create box3d.py Create box3d example Jan 29, 2021
@QuLogic QuLogic mentioned this pull request Jan 29, 2021
3 tasks
@jklymak
Copy link
Member

jklymak commented Feb 1, 2021

The aspect ratio has changed, but this now works with #19399

@QuLogic
Copy link
Member

QuLogic commented Feb 3, 2021

If you rebase on latest master, this should now work again on CI.

@iuryt
Copy link
Contributor Author

iuryt commented Feb 4, 2021

It looks like it failed for Tests / Python 3.8 on ubuntu-16.04 (pull_request).

@jklymak jklymak modified the milestones: v3.4.1, v3.4-doc Feb 4, 2021
Copy link
Member

@timhoffm timhoffm 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 the PR!

Generally, aim at making the example as small and concise as possible, both the description and the code. This will help people to understand it and get the relevant information and concepts used.

examples/mplot3d/box3d.py Outdated Show resolved Hide resolved
examples/mplot3d/box3d.py Outdated Show resolved Hide resolved
examples/mplot3d/box3d.py Outdated Show resolved Hide resolved
examples/mplot3d/box3d.py Outdated Show resolved Hide resolved
examples/mplot3d/box3d.py Outdated Show resolved Hide resolved
examples/mplot3d/box3d.py Outdated Show resolved Hide resolved
iuryt and others added 7 commits February 4, 2021 18:34
Change 'da' to 'data'

Co-authored-by: Tim Hoffmann <2836374+timhoffm@users.noreply.github.com>
Simplify definition for contourf key args

Co-authored-by: Tim Hoffmann <2836374+timhoffm@users.noreply.github.com>
Compacting code

Co-authored-by: Tim Hoffmann <2836374+timhoffm@users.noreply.github.com>
Simplify the title of the tutorial

Co-authored-by: Tim Hoffmann <2836374+timhoffm@users.noreply.github.com>
Concise documentation

Co-authored-by: Tim Hoffmann <2836374+timhoffm@users.noreply.github.com>
Remove credits
Replace da for data
Copy link
Contributor Author

@iuryt iuryt left a comment

Choose a reason for hiding this comment

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

Replace 'da' for 'data'

examples/mplot3d/box3d.py Outdated Show resolved Hide resolved
examples/mplot3d/box3d.py Outdated Show resolved Hide resolved
examples/mplot3d/box3d.py Outdated Show resolved Hide resolved
iuryt and others added 2 commits April 23, 2021 14:18
Co-authored-by: Elliott Sales de Andrade <quantum.analyst@gmail.com>
@iuryt
Copy link
Contributor Author

iuryt commented Apr 23, 2021

@QuLogic
Sorry for the late response to this matter.
Let me know if this works.

examples/mplot3d/box3d.py Outdated Show resolved Hide resolved
Co-authored-by: Elliott Sales de Andrade <quantum.analyst@gmail.com>
@QuLogic QuLogic modified the milestones: v3.4-doc, v3.5.0 May 3, 2021
@QuLogic QuLogic merged commit f6790c0 into matplotlib:master May 3, 2021
@QuLogic
Copy link
Member

QuLogic commented May 3, 2021

Thanks @iuryt! Congratulations on your first PR to Matplotlib 🎉 We hope to hear from you again.

@iuryt
Copy link
Contributor Author

iuryt commented May 3, 2021

Thanks @QuLogic !
I was a long journey for a simple contribution, but I have learned a lot.
Hope I have other contributions to community.

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.

None yet

4 participants