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

Unit conversion of ut1_to_tt in cirs_to_itrs(), itrs_to_cirs(), tod_to_itrs(), and itrs_to_tod() #15

Closed
hannorein opened this issue Mar 23, 2024 · 4 comments · Fixed by #13
Assignees
Labels
bug Something isn't working
Milestone

Comments

@hannorein
Copy link
Contributor

I think there is another error related to the time calculation, specifically on this line in cirs_to_itrs. ut1_to_tt should not be subtracted from jd_tt_low.

hannorein added a commit to hannorein/SuperNOVAS that referenced this issue Mar 23, 2024
@attipaci
Copy link
Collaborator

attipaci commented Mar 23, 2024

Hi @hannorein,

Thanks for finding the bug. The line you pointed to is definitely not right, although for a slightly different reason than what you noted. Notice, that cirs_to_itrs() takes a TT-based JD as its input, but then needs to call cel2ter() with a UT1-based JD argument, which is why ut1_to_tt1 must be subtracted. However, there was indeed a bug, since ut1_to_tt is in seconds, whereas JD is in days. So correctly, it should be:

   cel2ter(jd_tt_high, jd_tt_low - ut1_to_tt / DAY, ...);

@attipaci
Copy link
Collaborator

attipaci commented Mar 23, 2024

The issue affects itrs_to_cirs(), tod_to_itrs(), and itrs_to_tod() also. I committed and pushed a different fix in the commit 63449f6. I hope that resolves it sufficiently, but let me know if you think there is more to be done.

@attipaci attipaci changed the title Error in cirs_to_itrs Unit conversion of ut1_to_tt argument in cirs_to_itrs(), itrs_to_cirs(), tod_to_itrs(), and itrs_to_tod() Mar 23, 2024
@attipaci attipaci pinned this issue Mar 23, 2024
@attipaci attipaci self-assigned this Mar 23, 2024
@attipaci attipaci added the bug Something isn't working label Mar 23, 2024
@attipaci attipaci added this to the 1.0.1 milestone Mar 23, 2024
@hannorein
Copy link
Contributor Author

I noticed the units don’t work out. But your are right about the UT vs TT

@attipaci attipaci linked a pull request Mar 23, 2024 that will close this issue
@attipaci attipaci changed the title Unit conversion of ut1_to_tt argument in cirs_to_itrs(), itrs_to_cirs(), tod_to_itrs(), and itrs_to_tod() Unit conversion of ut1_to_tt in cirs_to_itrs(), itrs_to_cirs(), tod_to_itrs(), and itrs_to_tod() Mar 23, 2024
@attipaci
Copy link
Collaborator

attipaci commented Mar 23, 2024

Given the severity of this bug, I also added the fix directly to the main branch.

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
2 participants