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

RA/DEC in Barycentric ICRS, not J2000 #160

Merged
merged 2 commits into from May 10, 2023
Merged

RA/DEC in Barycentric ICRS, not J2000 #160

merged 2 commits into from May 10, 2023

Conversation

sbailey
Copy link
Contributor

@sbailey sbailey commented May 10, 2023

This PR removes RA/DEC references to J2000, replacing them with more correct descriptions of barycentric ICRS. I also updated the standard column description file so that future updates will use the more correct description. I didn't change pre-existing descriptions that just said "Right Ascension" without mentioning J2000, since they are correct even if they are less complete than they could be (and we've got a lot of other more important EDR tasks on our plate).

fixes #147

@sbailey sbailey requested a review from weaverba137 May 10, 2023 21:51
@weaverba137
Copy link
Member

Looks like the tests for the column update code were relying on values in the csv file that need to be updated in the tests themselves. If you can fix those, I don't see any reason not to merge as soon as tests pass.

PS, while you're looking at desidatamodel, #157 just needs a review before merging.

@coveralls
Copy link

Coverage Status

Coverage: 100.0%. Remained the same when pulling 689ea16 on radec into b20e0a0 on main.

@sbailey sbailey merged commit 8a2af3d into main May 10, 2023
20 checks passed
@sbailey sbailey deleted the radec branch May 10, 2023 22:17
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.

Should RA, Dec descriptions include J2000?
3 participants