Skip to content

Rotate errorbar caps in polar plots #21006

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

Closed
wants to merge 1 commit into from

Conversation

dstansby
Copy link
Member

@dstansby dstansby commented Sep 6, 2021

PR Summary

Fixes #441 (what prize do I get for a three figure issue 😄?). Here's an example:
Figure_1

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

@dstansby dstansby marked this pull request as draft September 6, 2021 11:32
@dstansby dstansby force-pushed the polar-errcaps branch 3 times, most recently from 370b4a2 to dce4026 Compare September 6, 2021 17:07
@dstansby dstansby added this to the v3.6.0 milestone Sep 13, 2021
@dstansby dstansby marked this pull request as ready for review September 13, 2021 09:01
@anntzer
Copy link
Contributor

anntzer commented Sep 13, 2021

To decrease the image diff I think it's good enough just add a separate test just checking png (as we don't expect to have anything that's backend-dependent here) and leave test_errorbar as it was initially?

Otherwise looks good to me.

@dstansby
Copy link
Member Author

To decrease the image diff I think it's good enough just add a separate test just checking png (as we don't expect to have anything that's backend-dependent here) and leave test_errorbar as it was initially?

👍 - I thought it was better to modify or add to an existing image than create a new one, but I guess that's not the case?

@dstansby
Copy link
Member Author

Ah right, I get your point about just adding a new .png test instead of changing all three images 👍

@anntzer
Copy link
Contributor

anntzer commented Sep 13, 2021

Thanks.
Now that the figure is bigger, it appears that the caps for theta-errorbars are drawn radially at the position of the cap, but perhaps it would make more sense to draw them to always be perpendicular to the errorbar instead? (radial caps would make sense if we drew the theta errorbars as circular arcs, but that's not the case currently)
(I guess it's "mostly" a matter of storing somewhere the info so that when you apply the rotation to the cap, you know where the center is.)

@deep-jkl
Copy link
Contributor

Hi, would you kindly consider changes in #20914 ? It will allow you specify transform during construction and not by overwriting _transform afterwards. However, it is not reviewed and I don’t know if it is a good practice to chain PRs.

@dstansby
Copy link
Member Author

Thanks for bringing that up - I've had a look at that PR, and will re-base this one if/when #20914 is merged.

@jklymak
Copy link
Member

jklymak commented Oct 5, 2021

I'll mark draft until #20914 gets in...

@jklymak jklymak marked this pull request as draft October 5, 2021 07:46
@timhoffm
Copy link
Member

timhoffm commented Oct 5, 2021

#20914 is in. Ping @dstansby ready for rebase.

@tacaswell
Copy link
Member

This PR is affected by a re-writing of our history to remove a large number of accidentally committed files see discourse for details.

To recover this PR it will need be rebased onto the new default branch (main). There are several ways to accomplish this, but we recommend (assuming that you call the matplotlib/matplotlib remote "upstream"

git remote update
git checkout main
git merge --ff-only upstream/main
git checkout YOUR_BRANCH
git rebase --onto=main upstream/old_master
# git rebase -i main # if you prefer
git push --force-with-lease   # assuming you are tracking your branch

If you do not feel comfortable doing this or need any help please reach out to any of the Matplotlib developers. We can either help you with the process or do it for you.

Thank you for your contributions to Matplotlib and sorry for the inconvenience.

@dstansby
Copy link
Member Author

I'm going to close this, as I'm not enjoying trying to get my head round the logic in errorbar at the moment. I'll leave the branch though if anyone wants to pick this up and address #21006 (comment).

@oscargus
Copy link
Member

I take the liberty to reopen this and mark it as orphaned. May increase the chance of someone picking it up.

@oscargus oscargus reopened this Apr 14, 2022
@QuLogic QuLogic modified the milestones: v3.6.0, v3.7.0 Jul 5, 2022
@deep-jkl deep-jkl mentioned this pull request Aug 9, 2022
6 tasks
@dstansby
Copy link
Member Author

I'm going to close this in favour of #23592

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.

Polar plot error bars don't rotate with angle
8 participants