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

added colorbar label #4930

Merged
merged 16 commits into from Jan 31, 2021
Merged

added colorbar label #4930

merged 16 commits into from Jan 31, 2021

Conversation

jeffreypaul15
Copy link
Contributor

@jeffreypaul15 jeffreypaul15 commented Jan 29, 2021

Description

Fixes #4906
Adds label to colorbar aligning it vertically.
This would change the figure so new hashes will have to be generated if what I've done is right.

@jeffreypaul15 jeffreypaul15 requested a review from a team as a code owner January 29, 2021 19:17
@pep8speaks
Copy link

pep8speaks commented Jan 29, 2021

Hello @jeffreypaul15! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2021-01-31 18:00:36 UTC

@nabobalis nabobalis added the map Affects the map submodule label Jan 29, 2021
@nabobalis
Copy link
Contributor

How does it come out?

@jeffreypaul15
Copy link
Contributor Author

How does it come out?

image

Should I alter the size?

@nabobalis
Copy link
Contributor

Can we not move the label? Maybe at the top or bottom of the bar?

It looks a bit odd sticking out at the side.

Maybe others disagree.

@jeffreypaul15
Copy link
Contributor Author

jeffreypaul15 commented Jan 30, 2021

Can we not move the label? Maybe at the top or bottom of the bar?

It looks a bit odd sticking out at the side.

Maybe others disagree.

I suppose moving it to the bottom and increasing the size would make it better?

sunpy/map/mapbase.py Outdated Show resolved Hide resolved
@dstansby
Copy link
Member

I agree it looks odd, if there's an easy way to put it at the top I think that would work well.

@jeffreypaul15
Copy link
Contributor Author

I agree it looks odd, if there's an easy way to put it at the top I think that would work well.

image

Does this look better?

@nabobalis
Copy link
Contributor

Maybe the font should be same size as the tick labels?

@jeffreypaul15
Copy link
Contributor Author

Maybe the font should be same size as the tick labels?

image

Yeah, I thought so too. I can't seem to find the actual size used though. size=12 seems to best work with it.

Co-authored-by: Nabil Freij <nabil.freij@gmail.com>
@nabobalis
Copy link
Contributor

I feel like it looks better at the bottom.

@jeffreypaul15
Copy link
Contributor Author

I feel like it looks better at the bottom.

image

Is this alright?

@nabobalis
Copy link
Contributor

Fine by me, lets see if others agree.

sunpy/map/mapbase.py Outdated Show resolved Hide resolved
@dstansby
Copy link
Member

Yep, I think this looks great, modulo @nabobalis comments above 👍

@nabobalis
Copy link
Contributor

I suspect some figure tests will need updating due to the change as well.

@jeffreypaul15
Copy link
Contributor Author

I suspect some figure tests will need updating due to the change as well.

Would the tests need updating? I thought only the hashes would have to be changed.

@nabobalis
Copy link
Contributor

I suspect some figure tests will need updating due to the change as well.

Would the tests need updating? I thought only the hashes would have to be changed.

Just the hashes.

@jeffreypaul15
Copy link
Contributor Author

I suspect some figure tests will need updating due to the change as well.

Would the tests need updating? I thought only the hashes would have to be changed.

Just the hashes.

I'm not too sure on how to generate the hashes as the tox -e figure tests do not pass locally for me. (Similar issue when I was working on draw_quadrangle

@nabobalis
Copy link
Contributor

I suspect some figure tests will need updating due to the change as well.

Would the tests need updating? I thought only the hashes would have to be changed.

Just the hashes.

I'm not too sure on how to generate the hashes as the tox -e figure tests do not pass locally for me. (Similar issue when I was working on draw_quadrangle

The figure tests on circleci tell you the hashes before and after in the logs.

@nabobalis nabobalis added the Merge When CI Passes Hit that merge button when it's all green! label Jan 31, 2021
@nabobalis nabobalis removed the request for review from a team January 31, 2021 18:09
@nabobalis
Copy link
Contributor

Thanks for the PR @jeffreypaul15

@nabobalis nabobalis merged commit 7c32a6c into sunpy:master Jan 31, 2021
@nabobalis nabobalis removed Merge When CI Passes Hit that merge button when it's all green! backport 2.1 labels Jan 31, 2021
@sunpy sunpy deleted a comment from sunpy-backport bot Jan 31, 2021
@jeffreypaul15
Copy link
Contributor Author

Thanks for the PR @jeffreypaul15

Always fun contributing to Sunpy! 🚀

Kate028 pushed a commit to Kate028/sunpy that referenced this pull request Feb 4, 2021
Co-authored-by: Nabil Freij <nabil.freij@gmail.com>
@jeffreypaul15 jeffreypaul15 deleted the dev3 branch March 26, 2021 14:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
map Affects the map submodule
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add label to colorbar in map.peek()
4 participants