Conversation
Summary of ChangesHello @gwbres, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces two new tests designed to rigorously validate the relativistic corrections applied when converting between Terrestrial Time (TT) and Barycentric Dynamical Time (TDB) within the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #452 +/- ##
=======================================
Coverage 83.02% 83.02%
=======================================
Files 23 23
Lines 3570 3570
=======================================
Hits 2964 2964
Misses 606 606 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds two tests to verify the relationship between TDB and TT time scales. My review focuses on the correctness of these new tests.
In test_tdb_tt_distance, I found an issue with the assertion logic that checks if the difference is within the 1.6ms bound. I've suggested a more direct and correct way to perform this check.
More importantly, in test_tdb_tt_mean_is_zero, I've identified a critical flaw in the test logic, which you also suspected in the PR description. The test incorrectly calculates the mean of the absolute differences and asserts it to be zero, which can lead to a false positive and hide potential bugs in the time scale conversion. I've proposed a rewrite of this test to correctly verify that the TDB-TT difference oscillates around zero and that its mean is negligible, which will help in identifying if the difference is unexpectedly always zero.
Signed-off-by: Guillaume W. Bres <guillaume.bressaix@gmail.com>
gwbres
force-pushed
the
tdb_tt
branch
2 times, most recently
from
33e7966 to
7873833
Compare
|
|
||
| // The difference should not be consistently zero, it should oscillate. | ||
| assert!( | ||
| min < 0.0 && max > 0.0, |
There was a problem hiding this comment.
I would add a dbg!(min) and dbg!(max) here to check what the error is.
|
I didn't know that TT and TDB were supposed to be within 1.6 ms of each other. How big of an error are you seeing ? Could you try with the ET time scale since that's a simpler computation of TDB, and maybe it should also be with 1.6 ms ? If the test still fails, would it make sense to increase the granularity of the time series from one sample per year to one sample every 3-4 months? Maybe the oscillation happens at a higher frequency. |
That's the thing, I have not been able to observe a time difference between TT and TDB so far
I'm not familiar with ET (😅) I will look into it |
|
To be exact, 1.6ms is the amplitude of the oscillation of TT around TDB. As you know TDB is thoughtout at the mass center of the Sun. The variation between both timescales, comes from Earth rotation around the earth, and the "Shapiro" effect, due to variations in the gravitational interaction along the entire rotation. The definition of TDB (which is correctly implemented by this library) is the following, which is actually a convention not the true physical time. The true physical time, is this value + 0.5s per year (drift). By convention, we "cancel" this drift to keep TDB close to TT for practical reasons |
2 participants
Hello,
I'm trying to write one or two more tests for TDB and TT to demonstrate that everything works correctly.
TDB is TT with relativistic corrections, which are very small, so they should be very close but not the same.
I read that |TDB-TT| should be at most 1.6ms at all times.
Also, the expected behavior is that the distance should oscillate around zero, over the course of an entire year.
In the second test
test_tdb_tt_mean_is_zeroI can see that the gap between both is always zero, which is incorrect to me right ?That test also "passes", because the mean is then obviously zero.