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

Change coordinates when using peek() #450

Closed
wants to merge 14 commits into from
Closed

Change coordinates when using peek() #450

wants to merge 14 commits into from

Conversation

mjm159
Copy link

@mjm159 mjm159 commented May 3, 2013

Issue #388

I added some functionality that allows users to change the coordinates by setting the "coords" value to 'HG', 'HPC', or 'HCC' in the peek() method.

@ayshih
Copy link
Member

ayshih commented May 3, 2013

This is more for issue #376, and I see that I neglected to add a comment there about my proof-of-concept commit (ayshih@50cf1ff), which was only discussed on the mailing list.

Unless I'm missing something, your implementation assumes that the native coordinates of the map is helioprojective (HPC) coordinates, but maps could actually be in heliographic (HG) or heliocentric (HCC) coordinates (or more, really, but let's not worry about that right now). You ought to check the coordinate system of the map so that you know which WCS conversions to call.

@fmayer
Copy link
Member

fmayer commented May 3, 2013

Maybe also make it possible to register new coordinate systems a dictionary of sorts.

@mjm159
Copy link
Author

mjm159 commented May 6, 2013

@ayshih I tried to include a comment referencing your work, but I don't know where the best place to put it is, so I just included it in the code itself.

You're right, I was making an assumption that the native coordinates are in HPC. Where are the native coordinates determined? I spent a long time looking through the code trying to figure out where the initial coordinate system was set and didn't have much luck. When you wrote your proof of concept, how did you know to put it where you did?

I've heard the dictionary idea before and I actually really like it, and would prefer to use one. I would like to try to work with you guys to develop. Would it be best to make a dictionary of lamda functions?

@mjm159
Copy link
Author

mjm159 commented May 6, 2013

Also, I want to apologize for my poorly commented initial commits. I'm still getting the hang of the whole GIT experience :)

self.heliographic_longitude,
x, y)
elif coords == 'HPC':
coords = None
Copy link
Member

Choose a reason for hiding this comment

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

Why is this necessary?

@Cadair
Copy link
Member

Cadair commented Jun 10, 2013

Am I right in saying that this is assuming the coordinates of the input map is in HPC? If so this needs to be generalised :)

@ehsteve
Copy link
Member

ehsteve commented Jun 10, 2013

This code needs to check that the input coordinate system is HPC as it currently assumes this. Should be good to go after that change!

@ayshih ayshih mentioned this pull request Jul 31, 2013
@mjm159 mjm159 closed this Aug 1, 2013
@mjm159
Copy link
Author

mjm159 commented Aug 1, 2013

I'm closing this pull request until I get a chance to update the code

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature Request New feature wanted!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants