Merged
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR implements minor performance improvements in the Astronoby library by pre-computing values and refining algorithms. Key changes include:
- Replacing a dynamic method for nutation terms with a constant (NUTATION_TERMS) in nutation.rb.
- Enhancing the precise crossing finder in rising_transit_setting_events_calculator.rb with an adaptive binary search and improved linear interpolation.
- Introducing a new threshold constant (CROSSING_PRECISION_THRESHOLD) to guide the convergence of the crossing search.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| lib/astronoby/nutation.rb | Replaces the dynamic nutation_terms method with a static constant for faster access. |
| lib/astronoby/events/rising_transit_setting_events_calculator.rb | Improves the crossing finder logic using adaptive convergence and linear interpolation. |
Comments suppressed due to low confidence (3)
lib/astronoby/nutation.rb:254
- The 'nutation_terms' method is now redundant since the constant NUTATION_TERMS is used. Consider removing the method declaration to avoid any potential confusion or future misuse.
def nutation_terms
lib/astronoby/events/rising_transit_setting_events_calculator.rb:260
- For clarity and consistency, compute the relative altitude difference with explicit conversion to radians (e.g. pos_min[:altitude].radians - pos_min[:horizon].radians) to match the usage in the linear_interpolate call.
relative_min = pos_min[:altitude] - pos_min[:horizon]
lib/astronoby/events/rising_transit_setting_events_calculator.rb:256
- [nitpick] There appears to be duplicated interpolation logic between the inline approach in find_precise_crossing and this standalone method. Consider consolidating the interpolation functionality to avoid potential inconsistencies.
def interpolate_crossing(t_min, t_max)
Repository owner
deleted a comment from
codecov-commenter
Mar 28, 2025
Merged
rhannequin
added a commit
that referenced
this pull request
May 12, 2025
## 0.7.0 - 2025-05-12 _If you are upgrading: please see [UPGRADING.md]._ ### Bug fixes * Fix Moon monthly phase events calculation by @valeriy-sokoloff in ([#124]) ### Features * Add `Instant` value object ([#121]) * Introduce barycentric position of Solar System major bodies ([#127]) * Introduce Astrometric position for planets ([#129]) * Rename Barycentric into Geometric ([#130]) * Rename IRCF and remove module Position ([#131]) * Geometric and Astrometric reference frames with coordinates ([#132]) * Ecliptic coordinates for Geometric and Astrometric reference frames ([#134]) * Add Geometric and Astrometric positions for `Sun` and `Moon` ([#135]) * Implement new aberration correction ([#136]) * Precession matrix for 2006 P03 model ([#137]) * Introduce `MeanOfDate` reference frame ([#138]) * New nutation model ([#141]) * Light deflection correction ([#142]) * Introduce `Apparent` reference frame ([#143]) * Introduce `Topocentric` reference frame ([#145]) * Improve Vector integration with value objects ([#146]) * Handle refracted topocentric horizontal coordinates ([#147]) * Add `#angular_diameter` to apparent and topocentric reference frames ([#149]) * Introduce new calculator for rising, transit and setting times ([#148]) * Clean code after Ephem refactoring ([#152]) * Improve `RisingTransitSettingEventsCalculator` ([#155]) * Simplify `RisingTransitSettingEventsCalculator` ([#156]) * Lazy-load reference frames ([#157]) * Overall performance improvements ([#163]) * Add support for IMCCE INPOP by @JoelQ and @rhannequin ([#166]) * Update INPOP excerpt in spec data ([#167]) * Introduce a better rise/transit/set calculator ([#168]) * Drop `Astronoby::Observer#observe` ([#174]) ### Improvements * Bump standard from 1.42.1 to 1.49.0 by @dependabot ([#123], [#128], [#150], [#165]) * Bump rubyzip from 2.3.2 to 2.4.1 by @dependabot ([#120]) * Add more tests for Julian Date conversion ([#122]) * Upgrade main Ruby version and supported ones ([#125]) * Update email address and gem description ([#126]) * Increase precision of mean obliquity ([#133]) * Add supported Rubies ([#139]) * Set Ruby 3.4.2 as default version ([#140]) * Fix dependency secutiry patch ([#151]) * Improve HMS/DMS formats ([#153]) * Use excerpts ephemerides for specs of Sun and Moon ([#154]) * Add link to deprecated documentation ([#160]) * Default Ruby 3.4.3 and support recent rubies ([#169]) * Better Moon phases test coverage ([#172]) * Optimize Observer with GMST from Instant ([#173]) * Update README about documentation location ([#175]) * Add GitHub Actions permissions ([#176]) ### New Contributors * @valeriy-sokoloff made their first contribution in #124 * @JoelQ made their first contribution in #166 **Full Changelog**: v0.6.0...v0.7.0 [#120]: #120 [#121]: #121 [#122]: #122 [#123]: #123 [#124]: #124 [#125]: #125 [#126]: #126 [#127]: #127 [#128]: #128 [#129]: #129 [#130]: #130 [#131]: #131 [#132]: #132 [#133]: #133 [#134]: #134 [#135]: #135 [#136]: #136 [#137]: #137 [#138]: #138 [#139]: #139 [#140]: #140 [#141]: #141 [#142]: #142 [#143]: #143 [#145]: #145 [#146]: #146 [#147]: #147 [#148]: #148 [#149]: #149 [#150]: #150 [#151]: #151 [#152]: #152 [#153]: #153 [#154]: #154 [#155]: #155 [#156]: #156 [#157]: #157 [#160]: #160 [#163]: #163 [#165]: #165 [#166]: #166 [#167]: #167 [#168]: #168 [#169]: #169 [#172]: #172 [#173]: #173 [#174]: #174 [#175]: #175 [#176]: #176 [UPGRADING.md]: https://github.com/rhannequin/astronoby/blob/main/UPGRADING.md
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
These are just tiny adjustments that won't make the computations 10 times faster but that will at least optimize a bit the overall performance.
I run the following benchmark before and after each commit:
Before PR
After freezing nutation terms
After implementing a better crossing finder
After pre-computing leap seconds
Conclusion
As you can see, this is not a game changer and the library can still be considered as pretty slow, there are other significant optimizations that will need to be implemented in the future, but each change made some little progress.