-
Notifications
You must be signed in to change notification settings - Fork 0
Fix type-checking errors for workflow #72
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
base: pegasus
Are you sure you want to change the base?
Conversation
Summary of ChangesHello @lispandfound, 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 resolves several type-checking errors and a logical bug across various workflow scripts. The changes improve the robustness and accuracy of the workflow by correctly handling different source types during SRF generation, ensuring precise area calculations for point sources, and addressing type compatibility issues with external libraries. 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request addresses several type-checking errors and a bug in area calculation. The changes look good overall. In generate_velocity_model_parameters.py and realisation_to_srf.py, the handling of different source types (Fault, Plane, Point) is now more robust, fixing potential AttributeErrors. The correction to the fault area calculation in realisation_to_srf.py for point sources is also a good fix. The addition of type: ignore in merge_ts.py is a reasonable temporary solution for upstream type hint issues. I have one suggestion in plan_workflow.py to use a more idiomatic and efficient way to copy a dataclass instance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR fixes type-checking errors in the workflow codebase, specifically addressing issues in SRF generation, velocity model parameter generation, workflow planning, and timeslice merging.
- Fixed type-checking failures when handling different source types (Fault, Plane, Point) by adding proper isinstance checks and using appropriate properties
- Corrected the area calculation for point sources from
(length_m / 1000) ** 2tolength * width - Improved the Stage.parent property implementation to use deepcopy for better maintainability
- Added type: ignore comments for qcore type annotation issues that will be resolved upstream
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| workflow/scripts/realisation_to_srf.py | Added Fault import and isinstance checks to properly handle multi-plane faults vs single planes; fixed point source area calculation to multiply length by width instead of squaring length |
| workflow/scripts/plan_workflow.py | Improved Stage.parent property to use deepcopy instead of manual instantiation for better maintainability |
| workflow/scripts/merge_ts.py | Added type: ignore comments for tobytes() calls pending qcore type annotation updates |
| workflow/scripts/generate_velocity_model_parameters.py | Changed from computing mean dip across fault planes to using fault.dip property directly for type-checker compatibility |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Failing test case is because of a bug in |
|
Type check will fail until my PR to scipy is merged: scipy/scipy-stubs#992 |
|
I've decided to ignore the scipy type bug, with a TODO to remove this after scipy-stubs updates. |
This reverts commit f572768.
Fixes a few bugs spotted by the type-checker: