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

ImageAnimator axis_ranges refactor 2 #2867

Merged
merged 40 commits into from Mar 13, 2019

Conversation

Projects
None yet
5 participants
@DanRyanIrish
Copy link
Member

commented Dec 13, 2018

This PR replaces PR #2618 and addresses issue #2581.

The new API requires axis_ranges be given as None, [min, max], or pixel edges (number of pixels along axis + 1). It also updates slider value labels based on the user supplied axis_ranges.

@pep8speaks

This comment has been minimized.

Copy link

commented Dec 13, 2018

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

Line 513:9: E731 do not assign a lambda expression, use a def
Line 514:11: E127 continuation line over-indented for visual indent
Line 522:9: E731 do not assign a lambda expression, use a def
Line 557:21: E117 over-indented
Line 609:1: W293 blank line contains whitespace
Line 613:1: W293 blank line contains whitespace

Line 64:27: E231 missing whitespace after ','

Comment last updated at 2019-03-13 15:24:34 UTC
@sunpy-bot

This comment has been minimized.

Copy link

commented Dec 13, 2018

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

@DanRyanIrish DanRyanIrish requested a review from Cadair Dec 13, 2018

@Cadair Cadair added this to the 1.0 milestone Dec 13, 2018

@Cadair
Copy link
Member

left a comment

@DanRyanIrish This is awesome!! So much better in a number of places!

Show resolved Hide resolved examples/plotting/LineAnimator_examples.py Outdated
Show resolved Hide resolved sunpy/visualization/animator/base.py Outdated
Show resolved Hide resolved sunpy/visualization/animator/base.py
Show resolved Hide resolved sunpy/visualization/animator/base.py
Show resolved Hide resolved sunpy/visualization/animator/base.py Outdated
Show resolved Hide resolved sunpy/visualization/animator/base.py
Show resolved Hide resolved sunpy/visualization/animator/image.py Outdated
Show resolved Hide resolved sunpy/visualization/animator/line.py Outdated
Show resolved Hide resolved sunpy/visualization/animator/base.py
@Cadair

Cadair approved these changes Dec 13, 2018

@DanRyanIrish DanRyanIrish force-pushed the DanRyanIrish:line_animator_update_axis_range_2 branch from c151f2c to 0680b5f Dec 14, 2018

DanRyanIrish added a commit to DanRyanIrish/sunpy that referenced this pull request Dec 14, 2018

@DanRyanIrish DanRyanIrish force-pushed the DanRyanIrish:line_animator_update_axis_range_2 branch from bf5813e to 7f5a8d6 Dec 14, 2018

@DanRyanIrish

This comment was marked as outdated.

Copy link
Member Author

commented Dec 14, 2018

@Cadair @nabobalis Is this test fail a problem? If so, why did it fail?

https://dev.azure.com/sunpy/sunpy/_build/results?buildId=66

@DanRyanIrish

This comment was marked as outdated.

Copy link
Member Author

commented Dec 14, 2018

Also, is there a known issue with the Python 3.7 online tests? https://travis-ci.org/sunpy/sunpy/jobs/467842608

@nabobalis

This comment was marked as outdated.

Copy link
Contributor

commented Dec 14, 2018

There is something off with our Travis Online Build. For now you want to check Azure's one.

That one failed but we know its a database.rst fail which is known and fixed in other PRs and it looks like a 404 test for EVE.

None of this code changes any online tests so we can/could ignore it for this PR.

Also I am unsure how I didn't notice your last comment before that one. Sorry!

@DanRyanIrish

This comment was marked as outdated.

Copy link
Member Author

commented Dec 14, 2018

OK. SO then this PR is good to merge?

@nabobalis nabobalis force-pushed the DanRyanIrish:line_animator_update_axis_range_2 branch from 6a24a05 to a898ffc Mar 6, 2019

Changed functions defined in _sanitize_axis_ranges to lambda statemen…
…ts and other minor changes suggested by code review.
@nabobalis

This comment has been minimized.

Copy link
Contributor

commented Mar 13, 2019

I think we can merge this now.

@DanRyanIrish Can you just check on issue #2617?

@DanRyanIrish

This comment has been minimized.

Copy link
Member Author

commented Mar 13, 2019

I just need to push some minor new changes. And yes, I can check that issue.

@DanRyanIrish

This comment has been minimized.

Copy link
Member Author

commented Mar 13, 2019

@nabobalis I left an explanation of this PR's impact on #2617. #2617 (comment)

@DanRyanIrish

This comment has been minimized.

Copy link
Member Author

commented Mar 13, 2019

I agree @nabobalis that, assuming the tests pass (except the coverage ones!), this is now ready to merge.

@nabobalis

This comment has been minimized.

Copy link
Contributor

commented Mar 13, 2019

Thanks @DanRyanIrish. I will merge then the CI passes.

@DanRyanIrish

This comment has been minimized.

Copy link
Member Author

commented Mar 13, 2019

👍

@nabobalis

This comment has been minimized.

Copy link
Contributor

commented Mar 13, 2019

Online test due to timeout and conda build is hanging on all CI right now.

@nabobalis nabobalis merged commit 0677e06 into sunpy:master Mar 13, 2019

12 of 16 checks passed

codecov/patch 16.94% of diff hit (target 85.29%)
Details
sunpy.sunpy (Linux_37_online) Linux_37_online failed
Details
sunpy.sunpy Build #20190313.24 has test failures
Details
sunpy.sunpy (Linux_36_Conda_offline) Linux_36_Conda_offline was canceled
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-install Your tests passed on CircleCI!
Details
codecov/project 85.35% (+0.05%) compared to 572304b
Details
giles Click details to preview the documentation build
Details
sunpy-bot All checks passed
sunpy.sunpy (Linux_36_offline) Linux_36_offline succeeded
Details
sunpy.sunpy (Windows_36_offline) Windows_36_offline succeeded
Details
sunpy.sunpy (macOS_37_offline) macOS_37_offline succeeded
Details
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.