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

Use Frame methods to compute overlaps; remove current resampling.py #219

Closed
pmelchior opened this issue Oct 30, 2020 · 4 comments
Closed
Assignees

Comments

@pmelchior
Copy link
Owner

Originally posted by @herjy in #217 (comment)

Current methods in resampling.py are largely superseded by WCS transformations in Frame. The only missing methods is match_patches, which could easily be ported to Frame as well as Frame.intersect(other_frame).

@pmelchior
Copy link
Owner Author

pmelchior commented Nov 5, 2020

Here is an associated problem I stumbled over: why do frames have boxes at all, instead of just shapes?

Here's the problem. When we define a model frame with an non-zero origin in its bbox (which we do e.g. for a model frame that is larger than any individual observation), we have an ambiguity in its pixel coordinates. It could be starting from zero (the indices of the model pixels), but it could also start from origin (in which case it refers to some observed images, which defined the WCS for the model frame). That ambiguity leads to trouble (which I have found out the hard way again). We currently assume the latter in some parts of the code, but we need the former in some other parts.

You can see the problem also if you consider cases with more than two images that are offset wrt each other, even at the same resolution. We cannot define the origin of the model frame in relation to two or more images.

So, in the multi-observation case, we can do two things instead of offsetting the model frame with a non-zero origin:

  1. In match, we shift the origin of the observation so that its pixels are now considered in the coordinate system of the model frame.
  2. We define a new WCS for model frame with the correct shape and reference position so that its pixels map correctly to the image that previously defined the WCS (typically one in high-resolution).

The first opton is the cheap fix. I prefer option 2 for two reasons: It yields a WCS we can report for the entire model. And it allows us to remove bbox from Frame to avoid the ambiguity entirely. Source models can have a bbox, because they are always defined in the coordinate system of the model frame (dah!), but Frame really shouldn't. They always cover all pixels (not a subset), and they have a known relation to the sky though their WCSs.

@herjy @fred3m Let me know what you think.

@pmelchior
Copy link
Owner Author

Actually, option 1 is only working with a lot of care because its pixels index is what is needed for its own WCS transformation if we ignore origin, but its relation to the model frame (which is computed by going through the WCS) require origin.

I think the only clean way is to let Frames have simple shapes, and define WCSs whenever we work with more than one observation.

pmelchior added a commit that referenced this issue Nov 5, 2020
properly use deconv detection image (#207); hacks for frame.bbox.origin (#219)
@fred3m
Copy link
Collaborator

fred3m commented Nov 11, 2020

After thinking this over I agree that in principle you are right that the Frame origin should always be at (0,0). How will the code be affected by this? Is your proposal to have a bbox parameter for Frame that returns Box(self.shape, origin=(0,0))?

@pmelchior
Copy link
Owner Author

I think Frame should only have a shape but not a bbox.

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

No branches or pull requests

3 participants