Event finding from scalar expressions#519
Conversation
Also Rename frame_from_uid to frame_info (better name, used in Python API) Kept the previous frame_from_uid as deprecated
TODO: - ReportEvents struct (as opposed to Report)
Sending this to Gemini for code review
Summary of ChangesHello @ChristopherRabotin, 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 significantly enhances the analysis capabilities by introducing a robust event-finding system based on scalar expressions. It allows for the definition and detection of various trajectory events, such as crossings or specific conditions, and provides tools to report these events and their characteristics. Additionally, it refactors internal frame information retrieval, expands the range of available scalar expressions, and introduces S-expression serialization for analysis configurations, making them more flexible and interoperable. 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
|
There was a problem hiding this comment.
Code Review
This pull request introduces a significant and powerful feature for event finding based on scalar expressions. The overall structure is well-designed, with clear separation of concerns into new modules for events, reports, and vector expressions. The refactoring of VectorExpr into its own file and the renaming of frame_from_uid to frame_info improve code organization and clarity.
I've identified a few areas for improvement, mainly concerning code duplication, potential panics from using unwrap(), and some minor code clarity issues. Addressing these will enhance the robustness and maintainability of this new functionality.
…was returning the wrong state Currently debugging event arcs for eclipse finding.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a significant new feature: the ability to find events using scalar expressions. This is a major enhancement to the analysis capabilities of the library. The changes include a new event finding module with search algorithms, serialization for analysis-related structs, and a large expansion of the ScalarExpr enum. My review focuses on the new logic. I've found a few critical issues, including a bug in a mathematical expression evaluation that could lead to a panic, and potential panics in the new event search algorithms due to incorrect error handling and indexing. I've also included some suggestions for refactoring to improve code maintainability.
27712ba to
a0b0615
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a significant and powerful new feature: the ability to find events using scalar expressions. This is a great addition to the analysis capabilities of the library. The introduction of S-expression serialization for reports and expressions is also a very useful feature for interoperability. The refactoring of the analysis module into smaller, more focused modules (event, report, search, vector_expr) greatly improves the structure and maintainability of the code. The consistent renaming of frame_from_uid to frame_info across the codebase is also a welcome improvement for API clarity.
My review includes a few suggestions to fix correctness issues, particularly around angle and time calculations, and some recommendations to improve maintainability by reducing code duplication and making error handling more robust. Overall, this is an excellent contribution.
Arcs are not very fast because they're so precise (uses the report_events_slow). Default precision for eclipse is 100 ms.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a powerful event-finding mechanism based on scalar expressions, which is a significant enhancement to the analysis capabilities of the library. The refactoring of frame_from_uid to frame_info improves API clarity, and the move of VectorExpr to its own module is a good structural improvement. The implementation of the Brent solver for event finding is also a solid choice for performance.
I've identified a few critical issues, mainly related to the incorrect use of trigonometric functions and potential panics from unwrap() calls. Addressing these will greatly improve the correctness and robustness of the new features. I've also found some high-severity issues regarding error handling and a logic bug in time calculation. Please review the detailed comments.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a powerful and flexible event-finding system based on scalar expressions, which is a significant enhancement to the analysis capabilities of the library. The overall architecture is well-thought-out, with clear separation of concerns into new modules like event, search, and report. The use of S-expressions for serialization is a great choice for this domain.
I've identified a few areas for improvement, including a critical bug in a mathematical expression and opportunities to enhance code clarity and robustness. Addressing these points will help solidify this excellent new feature.
BUG: LST from ANISE and manual with ScalrExpr no longer match It could be an error with the to_radians of the scalar expression (likely)
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a significant and powerful feature for event finding based on scalar expressions. The overall architecture is well-thought-out, with clear separation of concerns into modules for expressions, events, searching, and reporting. The ability to serialize and deserialize reports and expressions using S-expressions is a great addition for interoperability and persistence.
I've identified a few areas for improvement, including a critical bug in an angle calculation, a potential issue with a division-by-zero check, and opportunities to reduce code duplication and improve clarity in some of the new mathematical functions. Addressing these points will enhance the correctness and maintainability of this new functionality.
|
Added S-Expr for Events. Examples: APO S-EXPR: ((scalar Element . TrueAnomaly) (desired_value . 180.0) (epoch_precision . "100 ms") (value_precision . 0.01) (ab_corr))Eclipse S-EXPR: ((scalar SolarEclipsePercentage (eclipsing_frame (ephemeris_id . 301) (orientation_id . 1) (mu_km3_s2) (shape))) (desired_value . 99.9) (epoch_precision . "100 ms") (value_precision . 1.0) (ab_corr)) |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a powerful event-finding capability based on scalar expressions. The implementation is extensive, adding new modules for events, searching, and reporting, and significantly expanding the ScalarExpr enum. The code is well-structured, but there are a few areas for improvement. My review focuses on improving maintainability by reducing code duplication, enhancing the robustness of angle-based event detection, addressing a potential performance issue in arc finding, and improving the clarity of a mathematical formula. Overall, this is a great addition to the library.
Summary
Add the ability to find events of any kind as long as they can be represented by the scalar expressions.
TODO: description + bug fixes in the tests. Currently submitting for Gemini code review.
Architectural Changes
No change
New Features
No change
Improvements
No change
Bug Fixes
No change
Testing and validation
Detail the changes in tests, including new tests and validations
Documentation
This PR does not primarily deal with documentation changes.