Conversation
Summary of ChangesHello @Ayushi-EQ, 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 integrates several new and updated New Zealand Velocity Model (NZVM) tomography models, specifically 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
|
![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 several new tomography models (EP2017, EP2022) and updates existing ones (EP2010, EP2020, EP2025) in the nzcvm_registry.yaml file. It also adds the corresponding HDF5 data files, which are tracked with Git LFS.
My review focuses on the changes in nzcvm_registry.yaml. I've left a few comments regarding formatting inconsistencies and a potential issue with an invalid URL value. Addressing these will improve the consistency and robustness of the configuration file.
A few additional points:
- The file
tomography/EP2022/EP2022_Merge.h5is added to the repository but doesn't seem to be referenced innzcvm_registry.yaml. If this file is not used, it might be worth removing it to keep the repository clean. - The old data files (
ep2010.h5,ep2020.h5,ep2025.h5) are now unreferenced in the registry but are not removed from the repository in this PR. If they are no longer needed, they should probably be removed in a future cleanup effort.
nzcvm_registry.yaml
Outdated
| title: New Zealand Wide model 2.2 seismic velocity and Qs and Qp models for New Zealand | ||
| url: n/a | ||
| title: New Zealand Wide model 3.1 seismic velocity and Qs and Qp models for New Zealand | ||
| url: Through Email |
There was a problem hiding this comment.
The value 'Through Email' is not a valid URL. This could cause issues for any automated scripts or tools that consume this file and expect a URL. If a URL is not available, consider omitting the url key entirely for this entry, or using a standardized placeholder that your tools can handle, like null or an empty string.
url: 
There was a problem hiding this comment.
"Personal communication (pending publication)" could be more formal. You could request Donna and get a URL.
nzcvm_registry.yaml
Outdated
| title: New Zealand Wide model 2.2 seismic velocity and Qs and Qp models for New Zealand | ||
| url: n/a | ||
| title: New Zealand Wide model 3.1 seismic velocity and Qs and Qp models for New Zealand | ||
| url: Through Email |
There was a problem hiding this comment.
"Personal communication (pending publication)" could be more formal. You could request Donna and get a URL.
nzcvm_registry.yaml
Outdated
| - name: EP2010 | ||
| elev: [ 15, 1, -3, -8, -15, -23, -30, -38, -48, -65, -85, -105, -130, -155, -185, -225, -275, -370, -620, -750 ] | ||
| path: tomography/EP2010/ep2010.h5 | ||
| path: tomography/EP2010/EP2010_New.h5 |
There was a problem hiding this comment.
Let's make the name a bit more descriptive.
Also we need README.md for each of EP models describing the data set
- Increase upper bound for vp from 10.0 to 11.0 - Increase upper bound for vs from 6.0 to 6.5 - Add note clarifying values are not physically derived
- Add Basin, Vs30, and Submodel TypedDicts for schema validation - Parametrize tests for basin, vs30, and submodel entries from registry - Add tests for basin boundaries, surfaces, smoothing, and containment - Add tests for vs30 file existence, HDF5 validity, and gridpoint checks - Add tests for submodel data existence and content validation - Improve test coverage and robustness for registry-driven datasets
…ntry - Update Canterbury basement file paths to use correct lowercase naming - Fix spacing in Wellington basement file path - Remove invalid Gisborne basement image reference from registry
…tion - expand elevation arrays to multiline lists for all tomography models - align indentation and list formatting throughout basin and submodel sections - fix inconsistent spacing and minor alignment issues in nested structures - improve overall YAML readability without changing data content
- Remove duplicate and unused imports in test_registry.py - Decorate test_nzcvm_registry_schema with @no_type_check for type checking bypass - Eliminate redundant and commented-out test code for basin smoothing boundaries - Remove unused variables and streamline vs30 tests for readability
|
Type checking and ruff will fail because they check @sungeunbae's tools code for the time being. We can refactor that stuff later. |
|
|
|
|
|
…phy section - Remove leftover conflict markers from EP2022 and EP2025 entries - Ensure consistent formatting for elevation arrays and paths - Clean up whitespace for improved readability
- Check now asserts the basin *boundary* contains the smoothing boundary, instead of the basin *polygon* (which includes the interior).
|
🎉 |
|
|
3 participants
Adding this new branch for a unified grid approach for several tomography models. Kindly review this.