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

Review of afw DM-2307: Extend QuadrupoleKey to celestial coords. #20

Merged
merged 1 commit into from Apr 12, 2015

Conversation

TallJimbo
Copy link
Member

No description provided.

PIXEL,
CELESTIAL
};

Copy link
Member Author

Choose a reason for hiding this comment

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

Could this be an enum class instead? It looks like gcc 4.4 supports those, though I'm not sure if Swig does. Looks like it's worth just trying it; your unit tests should be sufficient to see if it works.

@TallJimbo
Copy link
Member Author

I think this approach of modifying QuadrupoleKey to support celestial coordinates is the right idea in general (and sorry I didn't see your request for comments on this on HipChat earlier). But I think that even in celestial coordinates we want to just continue calling the fields "xx, yy, xy", and instead just change the units and note that x=ra, y=dec in the documentation in addFields. That may be inconsistent with how we've switched to "ra, dec" for centroids, but the reality is that no one uses "RaRa" or "DecDec", and I think most users will be quite comfortable just using xx, yy, xy here. I think that should cut down slightly on the changes you need to make here - the constructor from SubSchema will no longer need an extra argument.

We can thereby use the same underlying mechanism to describe quadrupoles in
pixel and celestial coordinates. Internally, we continue to refer to "x" and
"y", but use appropriate names and units when writing to a table.
@jdswinbank jdswinbank merged commit dce194d into master Apr 12, 2015
@ktlim ktlim deleted the tickets/DM-2307 branch August 25, 2018 06:44
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

2 participants