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

Transform #126

Merged
merged 8 commits into from Jul 8, 2014
Merged

Transform #126

merged 8 commits into from Jul 8, 2014

Conversation

crawfordsm
Copy link
Member

closes #67 and closes #66. Adds transformations of the CCDData object although uncertainties may not be handled correctly. It also handles rebinning of ccddata objects.

"""Transform the image

Using the function specified by transform_func, the transform will
be applied to all planes in ccd.
Copy link
Member

Choose a reason for hiding this comment

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

By all planes you mean image, mask and uncertainty, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed

@mwcraig
Copy link
Member

mwcraig commented Jul 8, 2014

Several specific comments are above -- this seems like a solid start.

A few broad questions:

  • Should rebin be doing some kind of interpolation?
  • [May be broader than this PR, really] Should we even try to transform the uncertainties and masks? With the uncertainties in particular, we are not changing the class of the uncertainty even though we are changing the uncertainty array to something that may no longer be of that class. I think it would be safer to raise an error if the data has an uncertainty.
  • Should units also be transformed? If so, it can be done pretty easily by calling the transform function with a Quantity like 1 * ccd.unit (almost all numpy functions work well with Quantity but not all work with a bare Unit).
  • Could you add mentions of these to the appropriate places in the docs?

@crawfordsm
Copy link
Member Author

As for transforming uncertainties and masks, there are a small set of cases where they will be handled fine (integer shifts) and then a much larger set of cases where they will have problems (just about anything else). Masks I think are handled correctly, but with the statement that anything that once had a bad pixel in it, now is a bad pixel. That is at least safe even though a more complex algorithm may still be able to handle and still include those pixels.

However, I don't think there is anything in existence right now that would handle transforming uncertainty frames correctly and the correlation between the pixels. This will probably be close for most cases, but I think this would be an issue to be improved upon in future versions.

@crawfordsm
Copy link
Member Author

As for Quantity, these should only be transformations in terms of geometric transforms. I think the user should do something else if they want to do transformations of the units. I've added a note as such.

@mwcraig
Copy link
Member

mwcraig commented Jul 8, 2014

It is getting increasingly tempting to not deal with uncertainties at all :) -- it will be useful to hear from users what they want/need.

I thought it was likely the case that transforms should be purely geometrical -- sounds good!

crawfordsm added a commit that referenced this pull request Jul 8, 2014
@crawfordsm crawfordsm merged commit 7071a58 into astropy:master Jul 8, 2014
@crawfordsm crawfordsm deleted the transform branch July 8, 2014 17:49
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.

ccdproc.transform ccdproc.rebin
2 participants