-
Notifications
You must be signed in to change notification settings - Fork 3
DM-53499: Use stellar motion catalog to correct positions #447
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
Conversation
| dimensions=("instrument", "skymap", "tract", "physical_filter"), | ||
| multiple=True, | ||
| astrometricCorrectionCatalog = ct.Input( | ||
| doc="Table of associated sources", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could this doc be clearer? I'm not entirely certain what associated sources means.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is actually a copy-paste error. I changed it to "Catalog with proper motion and parallax information."
| # in the join and avoid sorting. | ||
| dataWithMJD.sort("__index__") | ||
| medianMJD = astropy.time.Time(np.median(mjds), format="mjd", scale="tai") | ||
| mjds = visitTable.loc[dataWithPM["visit"]]["expMidptMJD"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably outside the scope of this ticket but can we one day get the visitTable to be astropy and remove the uses of pandas.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would do it if I had finished 12 hours earlier...I think we can get rid of the rest of the pandas pretty easily on another ticket now though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know that feeling, a future ticket would be great, thank you.
| "pmRA": "raPM", | ||
| "pmDec": "decPM", | ||
| "parallax": "parallax", | ||
| "isolated_star_id": "isolated_star_id", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this one need to be snake case when all of the others are camel case? Also above I think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Both the left and right hand sides have to match what is in existing tables.
| Catalog containing the epoch for the visit corresponding to the | ||
| isolatedSources. | ||
| astrometricCorrectionCatalog : `pd.DataFrame` | ||
| astrometricCorrectionCatalog : `astropy.table.Table` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These make me happy.
faa688d to
153a4ab
Compare
No description provided.