Refactor generate velocity model parameters#83
Conversation
…ions and point-source fixes
…ion logic to pure function - Move domain and duration computation into new generate_domain function - Add generate_domain_from_realisation as CLI entrypoint for file-based workflow - Improve separation of concerns and enable easier testing of domain logic
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 undertakes a significant refactoring of the simulation domain parameter generation process. The core functionality has been extracted into a new, dedicated module, enhancing modularity and maintainability. These changes improve the accuracy of seismic calculations, bolster test coverage, and provide more flexible interfaces for integrating domain generation into the workflow. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. 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
|
![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 is a significant and well-executed refactoring of the domain generation logic, moving it from generate_velocity_model_parameters.py to a new, more modular generate_domain.py script. The introduction of generate_domain and generate_domain_from_realisation functions is a great improvement for testability and reusability. Moving dict_zip to utils and adding comprehensive tests for it is also a positive change. The fixes to the rake and depth calculations are valuable improvements.
My review focuses on the new generate_domain.py script, where I've identified several opportunities for minor improvements, mainly around code clarity, removing redundancies, and handling edge cases. These include simplifying some data conversions, removing unnecessary type casts, and addressing a potential ValueError in average_rake. Overall, this is a high-quality contribution that significantly improves the codebase.
There was a problem hiding this comment.
Pull request overview
This PR refactors the generate_velocity_model_parameters stage by:
- Extracting and improving the
dict_ziputility function with enhanced type hints - Implementing moment-weighted circular mean for rake calculations
- Improving depth calculation logic to ensure it's at least as deep as the deepest source plus buffer
- Separating domain generation logic into reusable functions that don't require realisation files
Key changes:
- The
dict_zipfunction is moved toworkflow/utils.pywith comprehensive type overloads for better type inference - A new
generate_domain.pymodule replacesgenerate_velocity_model_parameters.pywith refactored functions - Unit tests added for
dict_zipand integration properties for domain generation
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 15 comments.
Show a summary per file
| File | Description |
|---|---|
| workflow/utils.py | Adds dict_zip function with type overloads for better type inference in 2 and 3 argument cases |
| workflow/scripts/generate_velocity_model_parameters.py | Entire file removed as functionality moved to generate_domain.py |
| workflow/scripts/generate_domain.py | New file containing refactored domain generation logic with separation of concerns between file I/O and domain calculations |
| tests/test_utils.py | Adds comprehensive test coverage for dict_zip function with various edge cases |
| pyproject.toml | Updates CLI command name from generate-velocity-model-parameters to generate-domain and adds test dependencies |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
…w into generate_domain_refactor
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…w into generate_domain_refactor
lispandfound
changed the title
generate_velocity_model_parameters stage:There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 14 changed files in this pull request and generated 9 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
lispandfound
changed the title
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…w into generate_domain_refactor
lispandfound
requested review from
AndrewRidden-Harper,
claudio525,
joelridden and
sungeunbae
claudio525
left a comment
claudio525
left a comment
There was a problem hiding this comment.
Nice one, just one minor comment.
|
|
||
| Environment | ||
| ----------- | ||
| Can be run in the cybershake container. Can also be run from your own computer using the `generate-domain` command which is installed after running `pip install workflow@git+https://github.com/ucgmsim/workflow`. |
There was a problem hiding this comment.
Nitpick, but maybe this should use pypi package instead of pulling it from github? Would need to do a release after this commit, but seems like the nicer option.
There was a problem hiding this comment.
At some point yes I will do this.
dict_zipfunction and moves it to utils. Doing this allowed proper testing of the function. I also added better type hinting in the two and three argument case.I also added some unit and integration tests to the generate_domain module, which test some key properties:
Finally, in response to @claudio525, I have extracted the domain generation logic into a function that does not require a realisation file. There is now
generate_domain.generate_domainwhich takes realisation objects and returns domain parameters, and generate_domain.generate_domain_from_realisation that takes a path to a realisation file and reads the relevant parameters from the file, and writes them back.