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

Add iterative wcs.all_world2pix #1281

Merged
merged 6 commits into from Jul 30, 2013
Merged

Conversation

@lpsinger
Copy link
Contributor

lpsinger commented Jul 23, 2013

Iterative inverse of wcs.all_pix2world. Closes #1066.

I also have a unit test in the works using some of the PTF example files that are already in astropy/astropy/wcs/tests/data, but my tests rely on #299/#1278.

Iterative inverse of wcs.all_pix2world. Closes #1066.
all_world2pix.__doc__ = """
Transforms world coordinates to pixel coordinates, using numerical
iteration to invert the method `~astropy.wcs.WCS.all_pix2world` within a
tolerance of 1e-6 pixels.

This comment has been minimized.

Copy link
@astrofrog

astrofrog Jul 24, 2013

Member

It might be worth making the tolerance a user-settable parameter which defaults to 1.e-6

This comment has been minimized.

Copy link
@lpsinger

lpsinger Jul 24, 2013

Author Contributor

Done. See 0486e5a.

raise ImportError(
"You must have Scipy installed to use this method. " +
"See <http://www.scipy.org>.")
pixes = []

This comment has been minimized.

Copy link
@astrofrog

astrofrog Jul 24, 2013

Member

Is there a reason for using pixes instead of pixels?

This comment has been minimized.

Copy link
@lpsinger

lpsinger Jul 24, 2013

Author Contributor

Yes: to mimic the method's name. See 6d39555.

@astrofrog

This comment has been minimized.

Copy link
Member

astrofrog commented Jul 24, 2013

This will be very useful, thanks!

lpsinger added 3 commits Jul 24, 2013
The name of this variable is meant to echo the name of the function,
world2pix.
@lpsinger

This comment has been minimized.

Copy link
Contributor Author

lpsinger commented Jul 26, 2013

The only Travis failures occur in the configurations where the optional dependencies are disabled. As intended, these produce the error message:

ImportError: You must have Scipy installed to use this method. See <http://www.scipy.org>.

Anything else needed for this PR?

@astrofrog

This comment has been minimized.

Copy link
Member

astrofrog commented Jul 26, 2013

@lpsinger - the tests that require scipy should be skipped for configurations where scipy is not installed. Check out how the tests in astropy.cosmology do it. At the end of the day, all configurations should pass.

@lpsinger

This comment has been minimized.

Copy link
Contributor Author

lpsinger commented Jul 27, 2013

@lpsinger - the tests that require scipy should be skipped for configurations where scipy is not installed. Check out how the tests in astropy.cosmology do it. At the end of the day, all configurations should pass.

Ah, of course. Fixed now.

@astrofrog

This comment has been minimized.

Copy link
Member

astrofrog commented Jul 28, 2013

@mdboom - just bringing this issue to your attention in case you can review it.

soln = scipy.optimize.broyden1(func, x0, x_tol=tolerance)
pix.append(soln.flatten())
return np.asarray(pix)
def all_world2pix(self, *args, **kwargs):

This comment has been minimized.

Copy link
@mdboom

mdboom Jul 28, 2013

Contributor

Minor nit. We should have a blank line before the beginning of a new method.

@mdboom

This comment has been minimized.

Copy link
Contributor

mdboom commented Jul 28, 2013

👍

@lpsinger

This comment has been minimized.

Copy link
Contributor Author

lpsinger commented Jul 30, 2013

Any other requests, or is someone ready to merge this?

mdboom added a commit that referenced this pull request Jul 30, 2013
Add iterative wcs.all_world2pix
@mdboom mdboom merged commit 443ecf4 into astropy:master Jul 30, 2013
1 check passed
1 check passed
default The Travis CI build passed
Details
@mdboom

This comment has been minimized.

Copy link
Contributor

mdboom commented Jul 30, 2013

I just merged this, and then realised too late that this should probably have an entry in whatsnew. It's a pretty major (and very useful) new feature and should be highlighted.

@lpsinger

This comment has been minimized.

Copy link
Contributor Author

lpsinger commented Jul 30, 2013

I just merged this, and then realised too late that this should probably have an entry in whatsnew. It's a pretty major (and very useful) new feature and should be highlighted.

Thank you, it's nice of you to say so. I'll start a whatsnew patch; let me know if you are already on this.

@lpsinger lpsinger deleted the lpsinger:iterative_all_world2pix branch Jul 30, 2013
@mdboom

This comment has been minimized.

Copy link
Contributor

mdboom commented Jul 30, 2013

Nope -- I was hoping you would write the entry if you can...

lpsinger added a commit to lpsinger/astropy that referenced this pull request Jul 30, 2013
@embray

This comment has been minimized.

Copy link
Member

embray commented Jul 31, 2013

Please also add a changelog entry for this if you could.

@lpsinger

This comment has been minimized.

Copy link
Contributor Author

lpsinger commented Jul 31, 2013

Done.

@lpsinger

This comment has been minimized.

Copy link
Contributor Author

lpsinger commented Jul 31, 2013

Please also add a changelog entry for this if you could.

@iguananaut, see #1307.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.