Add a test for Hexbin Linear #21562

llricci · 2021-11-07T23:20:01Z

PR Summary

This PR is to fix the issue #21165:

A new test were add in matplotlib/lib/matplotlib/tests, with name test_hexbin_linear;
Also added a image file into matplotlib/lib/matplotlib/tests/baseline_images/text_axes, this image has a use, it is the image that the test will compare with.
Do it by adding the following:

image_comparison(baseline_images=['hexbin_linear'],
                  extensions=['png'], style='mpl20')
def test_hexbin_linear():

    np.random.seed(19680801)
    n = 100000
    x = np.random.standard_normal(n)
    y = 2.0 + 3.0 * x + 4.0 * np.random.standard_normal(n)

    fig, ax = plt.subplots()
    h = ax.hexbin(x, y, marginals=True, reduce_C_function=np.sum)
    plt.colorbar(h)

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

github-actions

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.

jklymak · 2021-11-08T08:02:53Z

The new test is failing unfortunately. Please make sure you test with up-to-date matplotlib main on your machine first.

llricci · 2021-11-08T09:35:28Z

The new test is failing unfortunately. Please make sure you test with up-to-date matplotlib main on your machine first.

Yeah, sure I were going to do that.

llricci · 2021-11-08T19:04:52Z

@jklymak hey, Why the codecov/project/tests test are failing?

jklymak · 2021-11-08T19:06:48Z

New contributors don't get all tests run automatically, but that makes codecov fail (because some tests are not being run). Nothing to worry about. I manually started your remaining tests. Thanks!

llricci · 2021-11-08T19:07:51Z

my pleasure, thank you by your help!

lib/matplotlib/tests/test_axes.py

llricci · 2021-11-09T09:32:05Z

Now why this one test are failing?

jklymak · 2021-11-09T09:36:44Z

If you click through the failing test you can see that it was failing to connect to a server to upload artifacts, which happens sometimes, but is probably not this PRs fault.

llricci · 2021-11-09T09:37:49Z

That might be

llricci · 2021-11-09T15:40:24Z

@jklymak Can I left like that? With this test failed?

jklymak · 2021-11-09T15:41:41Z

The tests passed, it was just an upload of artifacts that failed, so I would say its fine.

llricci · 2021-11-09T15:42:35Z

Ok

llricci · 2021-11-09T19:13:27Z

@QuLogic I have already solved the issues you pointed out, thanks!

llricci · 2021-11-10T21:36:55Z

I just merged this branch with main, and the tests, that previously succeed in passing, now it is getting me these errors

QuLogic · 2021-11-10T22:24:49Z

If possible, it'd be preferred if you rebased but it looks like we'll have to squash merge to get rid of the duplicate images anyway, so that might not be necessary.

llricci · 2021-11-11T18:03:28Z

@QuLogic I rebased it, and I think it worked, the tests are now passing, but codecov/project/tests no, because as a new contributor, my tests did not run automatically

jklymak · 2021-11-11T19:38:04Z

Your PR has a bunch of extraneous commits, hence the PR cleanliness failure. I'm not sure if a squash merge will get rid of these, if I was fixing this I would do a rebase, drop the extra commits, and then rebase onto latest master. Back up your branch before you do this though so you don't lose anything! Maybe others will have better advice.

(back up means do git checkout fix-21165; git checkout -b tmp-backup; git checkout fix-21165 and then do the rebasing. If you don't know how to rebase, you will probably need to read a guide online, but git rebase i 12345678 where 12345678 is the last commit in git log that you did not author just before the first commit you did author.

llricci · 2021-11-11T20:48:50Z

@jklymak Apparently solved

llricci · 2021-11-12T01:32:35Z

How can I fix the fact that my baseline image is duplicated? I delete it in my local repo and push it? Any idea?

QuLogic · 2021-11-12T03:19:46Z

You almost had the rebase, but after rebasing, you have to force push up to GitHub. If you pull, you just get back the old commits again and merge them into the history again.

QuLogic · 2021-11-12T06:41:47Z

So I see that #21339 also exists, but has not been updated in some time. However, it was also blocked on #21349, so I think cannot merge this until that is fixed; is that right @timhoffm?

llricci · 2021-11-12T10:12:20Z

@QuLogic So I will have to do the rebase again? If so, could you write a small guide please? I've never done a rebase and the texts I found on internet is a little confusing.

jklymak · 2021-11-12T10:17:04Z

You did it right the first time, you just then undid your work by doing a git pull before force-pushing your changes to GitHub. (Also, please don't ask us to write unique guides for every PR... we have our own guide, listed in the docs, and there are lots of docs online. If you have a specific question about a step, feel free to ask on gitter, thanks for your understanding).

llricci · 2021-11-12T10:36:07Z

Ok, Sorry by the request, I am learning a lot contributing with open source software.

author Lucas Ricci <llricci@aluno.crb.g12.br> 1636671767 -0300 committer Lucas Ricci <llricci@aluno.crb.g12.br> 1636755318 -0300 parent 29a3707 author Lucas Ricci <llricci@aluno.crb.g12.br> 1636671767 -0300 committer Lucas Ricci <llricci@aluno.crb.g12.br> 1636755293 -0300 deleted and updated baseline image for test_hexbin_linear() Resolve issue #21165, Add Hexbin Linear test a little correction in the code update Update update update a few corrections Resolve issue #21165, Add Hexbin Linear test a little correction in the code Resolve issue #21165, Add Hexbin Linear test a little correction in the code update Update update update a few corrections removing a typo where hexbon_linear() were duplicated

llricci · 2021-11-13T12:05:22Z

The tests passed now! Great.

timhoffm · 2021-11-13T15:27:20Z

So I see that #21339 also exists, but has not been updated in some time. However, it was also blocked on #21349, so I think cannot merge this until that is fixed; is that right @timhoffm?

@QuLogic a bit confused by this and that, but yes, it is preferable if we can solve #21349 before we merge this pull request here.

llricci · 2021-11-13T15:46:40Z

@timhoffm So what is gonna be the approach in that case?

jklymak · 2021-11-14T11:59:32Z

Right, this PR is fine, but the marginals may change under #21339, so adding the image, and then re-adding it is unecessary churn in our repo. So I'll mark this as depending on another PR and move to draft for now. Thanks for your patience @LucasRicci

llricci · 2021-11-14T15:57:04Z

@jklymak Just for further information, what is the max period that a PR should be inactivity to be closed?

jklymak · 2021-11-14T17:16:46Z

Just subscribe to upstream issue and when it is merged you can move this one to "ready"

llricci · 2022-07-13T15:50:33Z

@timhoffm Moving this PR to ready since #21339 was merged

QuLogic · 2022-07-13T15:58:46Z

This appears to need a rebase now, though looking again, it appears that this duplicates #21339?

llricci · 2022-07-14T23:38:00Z

@QuLogic Idk, but if so, I'll close this PR

timhoffm · 2022-07-18T00:04:08Z

The test is already covered in #21339 now. @LucasRicci I'm sorry that there has been overlapping work, but the objective of #21339 changed after we realized that the behavior was in fact correct and a test was then added for this.

Anyway, thank you for the PR. Even though this was not merged, your contributions is valuable to bring the discussion forward.

github-actions bot reviewed Nov 7, 2021

View reviewed changes

jklymak marked this pull request as draft November 8, 2021 08:02

jklymak added the status: needs revision label Nov 8, 2021

llricci marked this pull request as ready for review November 8, 2021 19:27

QuLogic reviewed Nov 9, 2021

View reviewed changes

lib/matplotlib/tests/test_axes.py Outdated Show resolved Hide resolved

lib/matplotlib/tests/test_axes.py Outdated Show resolved Hide resolved

llricci requested a review from QuLogic November 9, 2021 00:32

jklymak approved these changes Nov 9, 2021

View reviewed changes

jklymak added topic: testing and removed status: needs revision labels Nov 9, 2021

jklymak added this to the v3.6.0 milestone Nov 9, 2021

anntzer mentioned this pull request Nov 10, 2021

Check for images added-and-modified in a same PR #21593

Merged

7 tasks

llricci changed the title ~~Resolve issue #21165, Add Hexbin Linear Test~~ Add a test for Hexbin Linear Nov 12, 2021

Lucas Ricci added 4 commits November 12, 2021 19:15

removing 2 duplicates of test_hexbin_linear()

645e20b

fixed a typo where I used the wrong function

f4641ab

adding a @ before image_comparison

e420514

llricci requested a review from jklymak November 13, 2021 14:30

jklymak added the status: waiting for other PR label Nov 14, 2021

jklymak marked this pull request as draft November 14, 2021 11:59

QuLogic modified the milestones: v3.6.0, v3.7.0 Jul 5, 2022

llricci marked this pull request as ready for review July 13, 2022 15:49

QuLogic added status: needs rebase and removed status: waiting for other PR labels Jul 13, 2022

timhoffm closed this Jul 18, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a test for Hexbin Linear #21562

Add a test for Hexbin Linear #21562

llricci commented Nov 7, 2021 •

edited

Loading

github-actions bot left a comment

jklymak commented Nov 8, 2021

llricci commented Nov 8, 2021

llricci commented Nov 8, 2021 •

edited

Loading

jklymak commented Nov 8, 2021

llricci commented Nov 8, 2021

llricci commented Nov 9, 2021

jklymak commented Nov 9, 2021

llricci commented Nov 9, 2021

llricci commented Nov 9, 2021

jklymak commented Nov 9, 2021

llricci commented Nov 9, 2021

llricci commented Nov 9, 2021

llricci commented Nov 10, 2021

QuLogic commented Nov 10, 2021 •

edited

Loading

llricci commented Nov 11, 2021

jklymak commented Nov 11, 2021

llricci commented Nov 11, 2021

llricci commented Nov 12, 2021

QuLogic commented Nov 12, 2021

QuLogic commented Nov 12, 2021

llricci commented Nov 12, 2021

jklymak commented Nov 12, 2021

llricci commented Nov 12, 2021

llricci commented Nov 13, 2021

timhoffm commented Nov 13, 2021

llricci commented Nov 13, 2021

jklymak commented Nov 14, 2021

llricci commented Nov 14, 2021

jklymak commented Nov 14, 2021

llricci commented Jul 13, 2022

QuLogic commented Jul 13, 2022 •

edited

Loading

llricci commented Jul 14, 2022

timhoffm commented Jul 18, 2022

Add a test for Hexbin Linear #21562

Add a test for Hexbin Linear #21562

Conversation

llricci commented Nov 7, 2021 • edited Loading

PR Summary

PR Checklist

github-actions bot left a comment

Choose a reason for hiding this comment

jklymak commented Nov 8, 2021

llricci commented Nov 8, 2021

llricci commented Nov 8, 2021 • edited Loading

jklymak commented Nov 8, 2021

llricci commented Nov 8, 2021

llricci commented Nov 9, 2021

jklymak commented Nov 9, 2021

llricci commented Nov 9, 2021

llricci commented Nov 9, 2021

jklymak commented Nov 9, 2021

llricci commented Nov 9, 2021

llricci commented Nov 9, 2021

llricci commented Nov 10, 2021

QuLogic commented Nov 10, 2021 • edited Loading

llricci commented Nov 11, 2021

jklymak commented Nov 11, 2021

llricci commented Nov 11, 2021

llricci commented Nov 12, 2021

QuLogic commented Nov 12, 2021

QuLogic commented Nov 12, 2021

llricci commented Nov 12, 2021

jklymak commented Nov 12, 2021

llricci commented Nov 12, 2021

llricci commented Nov 13, 2021

timhoffm commented Nov 13, 2021

llricci commented Nov 13, 2021

jklymak commented Nov 14, 2021

llricci commented Nov 14, 2021

jklymak commented Nov 14, 2021

llricci commented Jul 13, 2022

QuLogic commented Jul 13, 2022 • edited Loading

llricci commented Jul 14, 2022

timhoffm commented Jul 18, 2022

llricci commented Nov 7, 2021 •

edited

Loading

llricci commented Nov 8, 2021 •

edited

Loading

QuLogic commented Nov 10, 2021 •

edited

Loading

QuLogic commented Jul 13, 2022 •

edited

Loading