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

Implement Differential Rotation for Maps #1876

Closed
wants to merge 8 commits into from
Closed

Implement Differential Rotation for Maps #1876

wants to merge 8 commits into from

Conversation

gbear605
Copy link
Contributor

@gbear605 gbear605 commented Aug 11, 2016

Slightly modified version of #1530 (see comments).

Nice things but not needed before pulling:

  • Add the rotation time to the new map, maybe peek() has to be updated to show that time when plotted, maybe a new keyword in Map
  • Work out how to update the coordinates mostly in the case of diff-rotating a not full-disk image

Tasks before pulling:

  • Add unit test for _warp_sun

@Cadair
Copy link
Member

Cadair commented Aug 11, 2016

Thanks @gbear605

I would like to see the test for warp_sun before we merge this. I will let @dpshelio and @wafels comment on the other things in the todo list.

@wafels from #1530 I was not suggesting we move this functionality into sunpy.coordinates just that we use those objects to specify things like vstart and vend.

@Cadair Cadair added the map Affects the map submodule label Aug 11, 2016
Because they were in kwargs, they were being passed into convert_hg_hpc, which doesn't want to take in a vend or vstart.
@dpshelio
Copy link
Member

Thanks @gbear605!!! This is what I love from github, people can continue the work I've left half-way. I've been trying to figure out why my attempts for not full-disk was not working... but I think it's to do with some columns/row, if I'm right I will be able to update this soon.

@wafels
Copy link
Member

wafels commented Aug 31, 2016

Not sure how this code will work, or should work, with maps that have masks.

@wafels
Copy link
Member

wafels commented Aug 31, 2016

Also, handling of NaN's might be better handled by doing a first pass on the data itself, and then a second pass on a map that has 1's where all the NaN's are. NaN's in the warped image are located where pixels are non-zero.

@wafels
Copy link
Member

wafels commented Aug 31, 2016

The _to_norm function has some odd behavior. If the minimum value of the input array is less than zero, then the output array does have a minimum value of zero. But if the minimum value of the input array is more than zero, the minimum value in the output array is arr.min() / arr.max(). Surely the minimum value should be zero to be consistent. Also, nanmin and nanmax functions should be used - these automatically ignore NaN's in the input.

@wafels
Copy link
Member

wafels commented Aug 31, 2016

If you have nans and infs in your map, a masked map is probably the best way to inform further code about what to do. This moves the handling of nans and infs on to numpy, and the user. In this case, there should be documentation in this code to explain to the user that masked maps are the best way forward. However, there should also be warnings in the docstrings to explain what might happen should nans and infs be passed through the code.

@Cadair
Copy link
Member

Cadair commented Jan 6, 2017

@gbear605 are you going to have time to continue this? If not would you have time to pick it up @wafels?

@gbear605
Copy link
Contributor Author

gbear605 commented Jan 6, 2017 via email

@Cadair
Copy link
Member

Cadair commented Jan 6, 2017

no problem @gbear605. Thanks for letting us know, and starting it off.

@Cadair Cadair added the Needs Adoption PRs that were abandoned but should be picked up again and merged in label Jan 6, 2017
@wafels
Copy link
Member

wafels commented Jan 6, 2017 via email

@Cadair Cadair removed the Needs Adoption PRs that were abandoned but should be picked up again and merged in label Jan 6, 2017
@agneet42
Copy link
Contributor

agneet42 commented Mar 6, 2017

@Cadair if could any work be done upon this as this issue is related with the sunkit image project? If you could guide me through. Thanks.

@wafels
Copy link
Member

wafels commented Mar 6, 2017 via email

@agneet42
Copy link
Contributor

agneet42 commented Mar 6, 2017

Wafels, yes I'd want to.
Could you help me out? Thank you.

@wafels
Copy link
Member

wafels commented Mar 6, 2017 via email

@agneet42
Copy link
Contributor

agneet42 commented Mar 6, 2017

@wafels I'm on it.

@nitinkgp23
Copy link
Contributor

nitinkgp23 commented Mar 6, 2017

Hey @wafels , I have been pondering over this since days, and I had a few things to discuss with you.
First, why do we want to shift to using sunpy.coordinates? Is it just to maintain consistency, or is there any extra feature that we are trying to implement? As far as I could understand, the function takes inputs in plain astropy quantities and returns the same. Only 3 of astropy.coordinates classes are being used i.e. Angle , Latitude and Longitude, so are we only converting these to use sunpy.coordinates ?

Are we trying to implement support of any other frame, apart from helioprojective ?

What are we trying to achieve by converting it? Any extra implementation?

Is it going to help in some way to implement image warping?

@agneet42
Copy link
Contributor

agneet42 commented Mar 6, 2017

@wafels
Sir, if we take an input in the form of,

c = SkyCoord(xu.arcsec, yu.arcsec, frame='helioprojective')
then we can access x and y in solarpy.coordinates using c.Tx and c.Ty.
Would that be a plausible approach?

@agneet42
Copy link
Contributor

agneet42 commented Mar 7, 2017

@wafels could you check if what i suggested above would work to convert input and output data into sunpy coordinates?

@wafels
Copy link
Member

wafels commented Mar 7, 2017 via email

@agneet42
Copy link
Contributor

agneet42 commented Mar 8, 2017

@wafels I am working on this. Could we discuss this and MGN as part of the sunkit-image project, whenever you have the time to?

@stale stale bot added the Stale The bot will close this PR after 6 months label Aug 6, 2017
@stale
Copy link

stale bot commented Aug 6, 2017

This pull request has been automatically marked as stale because it has not had any activity for the past five months. It will be closed if no further activity occurs. If the ideas in this pull request are still worth implementing, please make sure you open an issue to keep track of that idea!

@prateekiiest
Copy link
Contributor

prateekiiest commented Dec 21, 2017

@wafels, anything that I can start working on from here ?

I had this one query regarding the input for solar_rotate_coordinate in the current code.
It takes SkyCoord as input and since its under astropy.coordinates.

So by using sunpy.coordinates only do we mean some thing like using this as an input

c = Helioprojective(-570*u.arcsec, 120*u.arcsec, obstime=obstime, observer=get_earth(obstime))
solar_rotate_coordinate(c, '2010-09-10 13:34:56')

instead of using the SkyCoord for the same input c = SkyCoord(-570*u.arcsec, 120*u.arcsec, obstime=obstime, observer=get_earth(obstime), frame=frames.Helioprojective) ?
and then solar_rotate_coordinate(c, '2010-09-10 13:34:56')

@nabobalis
Copy link
Contributor

Is the issue for sunpy.coordinates still open?

@wafels
Copy link
Member

wafels commented Mar 27, 2018 via email

@nabobalis nabobalis added Needs Adoption PRs that were abandoned but should be picked up again and merged in and removed Stale The bot will close this PR after 6 months Needs Adoption PRs that were abandoned but should be picked up again and merged in map Affects the map submodule labels Nov 14, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants