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

Example for ImageAnimatorWCS #2752

Merged
merged 2 commits into from Jan 30, 2019

Conversation

Projects
None yet
6 participants
@prateekiiest
Copy link
Contributor

commented Sep 14, 2018

Closes #2288

@pep8speaks

This comment has been minimized.

Copy link

commented Sep 14, 2018

Hello @prateekiiest! Thanks for updating the PR.

Cheers ! There are no PEP8 issues in this Pull Request. 🍻

Comment last updated on January 28, 2019 at 13:20 Hours UTC
@sunpy-bot

This comment has been minimized.

Copy link

commented Sep 14, 2018

Thanks for the pull request @prateekiiest! Everything looks great!

@nabobalis

This comment has been minimized.

Copy link
Contributor

commented Oct 2, 2018

@prateekiiest Do you have a short example and output you can show us?
@DanRyanIrish Is this something more along the lines you were after?

@prateekiiest

This comment has been minimized.

Copy link
Contributor Author

commented Oct 4, 2018

This one was the example
#2566 (comment)

@nabobalis

This comment has been minimized.

Copy link
Contributor

commented Oct 4, 2018

That looks good. Will need the two people who deal with viz to have a look at this but I see no issues.

@nabobalis nabobalis requested review from Cadair and DanRyanIrish Oct 4, 2018

@DanRyanIrish

This comment has been minimized.

Copy link
Member

commented Oct 11, 2018

Hi @prateekiiest. Thanks for not giving up on this! I'll give it a try with an example.

Can you quickly describe the API for this? Is it that you create an animation instance and then call plot_user_input manually? e.g.

>>> ani = ImageAnimatorWCS(data, wcs, unit_x_axis="nm", unit_y_axis="W")
>>> ani.plot_user_input(tick_spacing, x_tick_position, y_tick_position,  xlabel, ylabel)
@prateekiiest

This comment has been minimized.

Copy link
Contributor Author

commented Oct 14, 2018

yes @DanRyanIrish , that is the approach I came about

@nabobalis nabobalis added the [Review] label Jan 7, 2019

@nabobalis

This comment was marked as outdated.

Copy link
Contributor

commented Jan 9, 2019

Sorry @prateekiiest, I should have reviewed this again sooner.

@nabobalis

This comment was marked as outdated.

Copy link
Contributor

commented Jan 10, 2019

It looks better now. I would suggest two things if you can.

  1. Add a changelog entry
  2. Could you also add a gallery example that uses your comment from the previous pull request showcasing how this works for users?
@prateekiiest

This comment was marked as outdated.

Copy link
Contributor Author

commented Jan 10, 2019

In this PR only for changelog and example?

@nabobalis

This comment was marked as outdated.

Copy link
Contributor

commented Jan 10, 2019

Yes if you could add them to this pull request.

@prateekiiest
Copy link
Contributor Author

left a comment

commit msg should have been 2752, which is the changelog entry

@prateekiiest

This comment was marked as outdated.

Copy link
Contributor Author

commented Jan 10, 2019

I will add the docs and more changes to this basic example, may be tomorrow.

In the meantime @nabobalis let me know if the example works fine on your computer, since I am gettig the same overflow error on my laptop (Win) as before and I had crashes on VMware Ubuntu recently, so I cant test there also.

@nabobalis

This comment was marked as outdated.

Copy link
Contributor

commented Jan 10, 2019

Bar the two comments, the example still runs for me.

Show resolved Hide resolved examples/plotting/ImageAnimatorWCS_Example.py Outdated
Show resolved Hide resolved examples/plotting/ImageAnimatorWCS_Example.py Outdated
Show resolved Hide resolved examples/plotting/ImageAnimatorWCS_Example.py Outdated
Show resolved Hide resolved examples/plotting/ImageAnimatorWCS_Example.py Outdated
Show resolved Hide resolved sunpy/visualization/animator/image.py Outdated
Show resolved Hide resolved sunpy/visualization/animator/image.py Outdated
Show resolved Hide resolved examples/plotting/imageanimatorwcs.py Outdated
Show resolved Hide resolved examples/plotting/imageanimatorwcs.py Outdated
Show resolved Hide resolved sunpy/visualization/animator/image.py Outdated

old news mate

@nabobalis

This comment was marked as outdated.

Copy link
Contributor

commented Jan 25, 2019

Note to self: squash merge.

@Cadair

Cadair approved these changes Jan 25, 2019

@nabobalis nabobalis force-pushed the prateekiiest:patch-2 branch 2 times, most recently from 51b6c07 to d7a3293 Jan 25, 2019

@nabobalis

This comment has been minimized.

Copy link
Contributor

commented Jan 28, 2019

Sorry about all of this @prateekiiest, we had a discussion about the original issue and decided that actually we do not want a new function to allow users to do this. They should use the existing API that is a bit more verbose.

So with that in mind, we decided we wanted the example you created in the previous pull request to be added in full to our example gallery!

@nabobalis nabobalis changed the title Plotting Style Update for ImageAnimatorWCS Example for ImageAnimatorWCS Jan 28, 2019

@Cadair Cadair requested a review from DanRyanIrish Jan 28, 2019

@Cadair

This comment has been minimized.

Copy link
Member

commented Jan 28, 2019

solar_x.set_axislabel_position('t')

# Now we can change the spacing and we have to use `astropy.units` here.
solar_y.set_ticks(spacing=1*u.arcmin, color='black')

This comment has been minimized.

Copy link
@Cadair

Cadair Jan 28, 2019

Member

Super nitty nit pick, would we make this spacing a little larger to make the final plot look nicer?

This comment has been minimized.

Copy link
@nabobalis

nabobalis Jan 28, 2019

Contributor

Sure but I set this up for the next line. Just to show you can prevent overlapping.

This comment has been minimized.

Copy link
@nabobalis

nabobalis Jan 28, 2019

Contributor

I fixed the issue, I got the wrong axes out.

@nabobalis nabobalis force-pushed the prateekiiest:patch-2 branch 3 times, most recently from a3290a3 to 80c3a45 Jan 28, 2019

@nabobalis nabobalis force-pushed the prateekiiest:patch-2 branch from 80c3a45 to 2ae11f2 Jan 28, 2019

@nabobalis

This comment was marked as outdated.

Copy link
Contributor

commented Jan 28, 2019

I just noticed a few little issues I caused that I have fixed now. THE POWER OF THE AMEND

@prateekiiest

This comment has been minimized.

Copy link
Contributor Author

commented Jan 28, 2019

@nabobalis I just saw the commits. No problem at all 👍

@Cadair Cadair merged commit 0e762c1 into sunpy:master Jan 30, 2019

10 of 11 checks passed

sunpy.sunpy Build #20190128.11 has failed
Details
ci/circleci: 32bit Your tests passed on CircleCI!
Details
ci/circleci: egg-info-36 Your tests passed on CircleCI!
Details
ci/circleci: egg-info-37 Your tests passed on CircleCI!
Details
ci/circleci: figure-tests-36 Your tests passed on CircleCI!
Details
ci/circleci: html-docs Your tests passed on CircleCI!
Details
ci/circleci: pip Your tests passed on CircleCI!
Details
codecov/patch Coverage not affected when comparing fdac973...2ae11f2
Details
codecov/project 86.45% (-0.08%) compared to fdac973
Details
giles Click details to preview the documentation build
Details
sunpy-bot All checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.