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

Resolve confusion on x/y and array row/column #16

Open
svank opened this issue Mar 30, 2023 · 2 comments
Open

Resolve confusion on x/y and array row/column #16

svank opened this issue Mar 30, 2023 · 2 comments
Assignees
Labels
bug Something isn't working

Comments

@svank
Copy link
Contributor

svank commented Mar 30, 2023

CoordinateIdentifier is defined as storing x and y coordinates. After star identification, a bunch of CoordinateIdentifers are instantiated, with sep's x going into the y slot and vice-versa, so the x/y slots of CoordinateIdentifer are really being used as row/column slots. And indeed, when making the stellar cutouts, x is used for the vertical axis and y the horizontal axis.

I'm not sure what the ultimate goal is for CoordinateIdentifer, but I think either its fields should be renamed as row/column or similar, or the x and y fields should be used consistently as vertical and horizontal coordinates. (This bit me while visualizing my patches.)

@jmbhughes
Copy link
Member

I've waffled back and forth between whether to use a namedTuple with the CoordinateIdentifier versus just a regular tuple without named fields. Did you have a preference or an alternative suggestion?

You're right that we should clarify the convention and be consistent though. I'm fine with making it row and col.

@svank
Copy link
Contributor Author

svank commented Apr 4, 2023

One upside to using the namedTuple is that one doesn't have to memorize/look up the order of the elements in the tuple, and it cuts down on the use of magic numbers (e.g. xs = [k['x'] for k in cpc.keys()] vs. xs = [k[2] for k in cpc.keys()])

Changing the names to "row" and "col" might indeed be the easiest fix here.

@jmbhughes jmbhughes added the bug Something isn't working label Apr 6, 2023
@jmbhughes jmbhughes self-assigned this Jun 7, 2023
@jmbhughes jmbhughes changed the title Confusion on x/y and array row/column Resolve confusion on x/y and array row/column Aug 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants