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.

Sign up for GitHub

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

Jump to bottom

added tests for map sequence animator #5041

Closed
wants to merge 16 commits into from
Closed

added tests for map sequence animator #5041

wants to merge 16 commits into from

Conversation

jeffreypaul15
Copy link
Contributor

@jeffreypaul15 jeffreypaul15 commented Feb 27, 2021 •
edited

Description

Fixes #2296
Added tests for MapSequenceAnimator.
Figure hashes have yet to be added.

@jeffreypaul15 jeffreypaul15 requested a review from a team as a code owner February 27, 2021 22:47
@nabobalis nabobalis added map Affects the map submodule No Changelog Entry Needed Tests Affects tests in some measure labels Feb 28, 2021
@nabobalis
Copy link
Contributor

Looking at our codecov (you will need to login via github unfortunately): https://codecov.io/gh/sunpy/sunpy/src/7d2b42b73a4130846b32ba3c9ecb2b22390b8e89/sunpy/map/mapsequence.py
https://codecov.io/gh/sunpy/sunpy/src/7d2b42b73a4130846b32ba3c9ecb2b22390b8e89/sunpy/visualization/animator/mapsequenceanimator.py

If I am reading it right, in fact MapSequenceAnimator has at least some test coverage while the plot method for MapSequence does not.

So I think we should also add a figure test for MapSequence.plot while we are here.

nabobalis
nabobalis reviewed Mar 12, 2021
View reviewed changes
sunpy/visualization/animator/tests/test_mapsequence.py Outdated
Comment on lines 45 to 46
map_animator.updatefig(1, map_animator.im, map_animator.sliders[0]._slider)
return map_animator.fig
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel like this is possible to test without a figure.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I suppose, I'll make this change soon

@jeffreypaul15
Copy link
Contributor Author

Looking at our codecov (you will need to login via github unfortunately): https://codecov.io/gh/sunpy/sunpy/src/7d2b42b73a4130846b32ba3c9ecb2b22390b8e89/sunpy/map/mapsequence.py
https://codecov.io/gh/sunpy/sunpy/src/7d2b42b73a4130846b32ba3c9ecb2b22390b8e89/sunpy/visualization/animator/mapsequenceanimator.py

If I am reading it right, in fact MapSequenceAnimator has at least some test coverage while the plot method for MapSequence does not.

So I think we should also add a figure test for MapSequence.plot while we are here.

I shall test that at test_mapsequence.py then?

@nabobalis
Copy link
Contributor

Yeah.

@jeffreypaul15 jeffreypaul15 requested a review from a team as a code owner March 18, 2021 03:46
@pep8speaks
Copy link

pep8speaks commented Mar 18, 2021 •
edited

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

Line 6:101: E501 line too long (114 > 100 characters)

Line 199:1: W293 blank line contains whitespace
Line 199:1: W391 blank line at end of file

Line 18:101: E501 line too long (110 > 100 characters)
Line 19:101: E501 line too long (106 > 100 characters)
Line 27:101: E501 line too long (102 > 100 characters)

Comment last updated at 2021-03-22 10:20:58 UTC

@jeffreypaul15
Copy link
Contributor Author

@nabobalis I've updated the tests accordingly and added a test for plot. Let me know if this is fine

@nabobalis nabobalis removed request for a team March 22, 2021 09:22
@nabobalis
Copy link
Contributor

You need to rebase the PR and I think a trivial changelog entry makes sense.

@jeffreypaul15 jeffreypaul15 requested review from a team as code owners March 22, 2021 09:50
@jeffreypaul15 jeffreypaul15 reopened this Mar 22, 2021
@jeffreypaul15 jeffreypaul15 reopened this Mar 22, 2021
@jeffreypaul15 jeffreypaul15 force-pushed the sunpy-2296 branch 2 times, most recently from 82d2995 to 53e569d Compare March 22, 2021 10:10
@nabobalis nabobalis removed request for a team March 22, 2021 10:14
@jeffreypaul15
Copy link
Contributor Author

jeffreypaul15 commented Mar 22, 2021 •
edited

You need to rebase the PR and I think a trivial changelog entry makes sense.

Having a bit of trouble with this, I'm not too sure as to what's the need for rebase. Apologies for the mistakes here, I kinda messed up the rebasing

@nabobalis
Copy link
Contributor

You need to rebase the PR and I think a trivial changelog entry makes sense.

You need to rebase the PR and I think a trivial changelog entry makes sense.

Having a bit of trouble with this, I'm not too sure as to what's the need for rebase.

You can merge in master if it's easier. There is conflict with master and this needs to be addressed.

Shane Maloney and others added 16 commits March 22, 2021 15:46
Fix scraper issue with Windows and URL path separation (sunpy#5022)
bd69190
@Cadair
Update changelog post release (sunpy#5028)
5aacbe6
@kathivashkiv @Cadair
Add regular CI to run ASV for new commits and benchmarks (sunpy#4975)
a4f3651 
* Create a CI script to run ASV daily

* CI: write a script

* CI: update `cron`

* Add asv config file

* Add benchmarks dir

* CI: update 'cron'

* Ci: update 'cron'

* CI: change 'cron' to 7:00 UTC

* CI: add `--skip-existing-successful`option

* CI: typo fix

* Add  benchmarks file

* CI: tmp add on push activation

* Modify benchmark

* CI: final version

* Tox codestyle fix

* Update: change range to `v2.1.dev..`

Co-authored-by: Stuart Mumford <stuart@cadair.com>

* CI: typo fix and tmp on push start

* CI: delete debug step

Co-authored-by: Stuart Mumford <stuart@cadair.com>
@chouhanaryan @ayshih @nabobalis
Added an example for drawing rectangles on maps (sunpy#4528)
5017581 
Co-authored-by: Albert Y. Shih <ayshih@gmail.com>
Co-authored-by: Nabil Freij <nabil.freij@gmail.com>
@wtbarnes
MDI synoptic map does not need crder
655b681
@nabobalis
whatsnew 3.0
51102e8
@nabobalis
added support to more than one registry url for hec
6a1ec40
added tests for map sequence animator
986e2b7
style fixes
52ef603
added figure hashes
40f3e9d
undid change in setup to run figure tests locally
e2ae761
@jeffreypaul15
Update test_mapsequence.py
2a95d9e
@jeffreypaul15
Update test_mapsequence.py
6edf3d3
@jeffreypaul15
Update test_mapsequence.py
64ed916
removed figure test
4da9f24
added test for map_sequence.plot()
3f7ab20
@jeffreypaul15
Copy link
Contributor Author

You need to rebase the PR and I think a trivial changelog entry makes sense.

You need to rebase the PR and I think a trivial changelog entry makes sense.

Having a bit of trouble with this, I'm not too sure as to what's the need for rebase.

You can merge in master if it's easier. There is conflict with master and this needs to be addressed.

So what exactly would I have to do?

@jeffreypaul15 jeffreypaul15 reopened this Mar 22, 2021
@jeffreypaul15
Copy link
Contributor Author

Actually, I shall reopen this PR. Its a lot easier

@jeffreypaul15 jeffreypaul15 deleted the sunpy-2296 branch March 22, 2021 10:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dpshelio dpshelio dpshelio left review comments

@nabobalis nabobalis nabobalis left review comments

Assignees
No one assigned
Labels
map Affects the map submodule Tests Affects tests in some measure
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Mapcube animation is not tested
8 participants
@jeffreypaul15 @nabobalis @pep8speaks @dpshelio @Cadair @kathivashkiv @chouhanaryan @wtbarnes