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

Fix differential rotation of map #3149

Merged
merged 10 commits into from May 30, 2019

Conversation

Projects
None yet
5 participants
@wafels
Copy link
Member

commented May 29, 2019

Description

The current differential rotation warping of maps does not take in to account the location of the observer. This PR is an attempt to fix this.

Fixes #

@pep8speaks

This comment has been minimized.

Copy link

commented May 29, 2019

Hello @wafels! 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 2019-05-30 09:05:39 UTC
@sunpy-bot

This comment has been minimized.

Copy link

commented May 29, 2019

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

@nabobalis nabobalis added this to the 1.0 milestone May 29, 2019

@nabobalis nabobalis added the [BugFix] label May 29, 2019

@Cadair Cadair force-pushed the wafels:fixdrot2 branch from 9961a68 to f63f5b0 May 29, 2019

Cadair added some commits May 29, 2019

@Cadair Cadair force-pushed the wafels:fixdrot2 branch from cf92576 to cb282fe May 29, 2019

@Cadair

This comment has been minimized.

Copy link
Member

commented May 29, 2019

The notebook comparing this routine to the equivalent implementation in reproject is here: https://gist.github.com/Cadair/3606b5aa25059b688fca24e950cfb25a

@Cadair

This comment has been minimized.

Copy link
Member

commented May 29, 2019

@wafels let me know if you want to quickly call up and chat about this, or feel free to leave a review on here if you prefer.

Cadair added some commits May 29, 2019

@wtbarnes

This comment has been minimized.

Copy link
Member

commented May 30, 2019

This need not be resolved before 1.0, but it seems that, now with the fix to account for the different observer and the example provided in @Cadair's notebook, labeling this function as just differential_rotate is potentially a bit confusing for users.

Perhaps I'm confused, but this can now be used for arbitrary reprojection of a map to a new observer right? and differential rotation is just a special case of this in which the new observer's location is determined by the differential rotation of the Sun.

This is also related to the larger question of potential overlap with and dependence on the reproject package, but could this function here be renamed as something a bit more general (e.g. reproject_map) and then differential_rotate could be a wrapper around reproject_map that takes a time (or a time delta), constructs an observer based on that time, and passes it to reproject_map?

Show resolved Hide resolved changelog/3149.bugfix.rst Outdated
@wafels

This comment has been minimized.

Copy link
Member Author

commented May 30, 2019

Yes, differential_rotate can now be used for arbitrary re-projection (with the caveat that it is still experimental). I suppose you can look at the current differential_rotate as handling the general case when both the observer location and time have changed. Map re-projection could be a substantial project in a post 1.0 world?

@wafels

This comment has been minimized.

Copy link
Member Author

commented May 30, 2019

Also, +1 on the PR.

@Cadair

Cadair approved these changes May 30, 2019

@Cadair Cadair merged commit 2b26fad into sunpy:master May 30, 2019

16 checks passed

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/patch 100% of diff hit (target 90.3%)
Details
codecov/project Absolute coverage decreased by -<.01% but relative coverage increased by +9.69% compared to c597795
Details
giles Click details to preview the documentation build
Details
sunpy-bot All checks passed
sunpy.sunpy Build #20190530.5 succeeded
Details
sunpy.sunpy (Linux_36_Conda_offline) Linux_36_Conda_offline succeeded
Details
sunpy.sunpy (Linux_36_offline) Linux_36_offline succeeded
Details
sunpy.sunpy (Linux_37_online) Linux_37_online succeeded
Details
sunpy.sunpy (Windows_36_offline) Windows_36_offline succeeded
Details
sunpy.sunpy (macOS_37_offline) macOS_37_offline succeeded
Details

Cadair added a commit that referenced this pull request May 30, 2019

Merge pull request #3153 from wafels/remove_norm
Removal of image utils file due to un_norm and to_norm not being used after #3149

nabobalis added a commit to nabobalis/sunpy that referenced this pull request May 30, 2019

Merge pull request sunpy#3149 from wafels/fixdrot2
Fix differential rotation of map

nabobalis added a commit to nabobalis/sunpy that referenced this pull request May 30, 2019

Merge pull request sunpy#3153 from wafels/remove_norm
Removal of image utils file due to un_norm and to_norm not being used after sunpy#3149
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.