Skip to content

Align standards with the rest of the codebase#358

Closed
lispandfound wants to merge 46 commits intomasterfrom
type_checking
Closed

Align standards with the rest of the codebase#358
lispandfound wants to merge 46 commits intomasterfrom
type_checking

Conversation

@lispandfound
Copy link
Contributor

This PR deletes a lot of dead code used by the old workflow, deprecates some modules still used in gmhazard and cybershake investigation,
and reformats and reorganise the rest to comply with modern standards, i.e.

  • Ruff formatting,
  • Deptry dependency checks,
  • Pytest + coverage checks,
  • Ty type-checking,
  • Numpydoc validation.

@gemini-code-assist
Copy link
Contributor

Summary of Changes

Hello @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 represents a comprehensive effort to modernize and streamline the codebase. It focuses on removing legacy components, adopting contemporary development standards, and enhancing code quality through improved type hinting and documentation. The changes aim to make the project more maintainable, consistent, and easier to develop against, aligning it with current best practices in Python software engineering.

Highlights

  • Codebase Modernization: A significant amount of dead code related to old workflows and deprecated modules (e.g., MPI logging, old configuration systems, specific IM/SRF/VM handling) has been removed.
  • Dependency Management & Tooling: The pyproject.toml file has been updated to explicitly list dependencies and integrate modern development tools such as Ruff for formatting, Deptry for dependency checks, Pytest for testing with coverage, and Ty for static type-checking.
  • Code Quality & Documentation: Existing code has been reformatted to comply with Ruff standards, and numerous functions across various modules have received updated type hints and Numpydoc-style docstrings to improve clarity and maintainability.
  • Refactoring of Core Modules: Key modules like qcore/geo.py, qcore/coordinates.py, qcore/nhm.py, qcore/timeseries.py, and qcore/xyts.py have undergone substantial refactoring, including removal of obsolete functions and classes, and updates to function signatures and internal logic for better consistency and modern Python practices.
  • Testing Infrastructure: The testing directory structure has been reorganized (e.g., qcore/test moved to tests/), and test files have been updated or removed to align with the changes in the core codebase.
Ignored Files
  • Ignored by pattern: .github/workflows/** (5)
    • .github/workflows/deptry.yml
    • .github/workflows/numpydoc.yml
    • .github/workflows/pytest.yml
    • .github/workflows/ruff.yml
    • .github/workflows/types.yml
Using Gemini Code Assist

The 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 /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature Command Description
Code Review /gemini review Performs a code review for the current pull request in its current state.
Pull Request Summary /gemini summary Provides a summary of the current pull request in its current state.
Comment @gemini-code-assist Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help /gemini help Displays a list of available commands.

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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.

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

  1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution.

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This is a substantial and impressive pull request that significantly modernizes the codebase. The deletion of dead code, transition to pyproject.toml for dependency management, and widespread improvements in type hinting and documentation are all excellent changes that will greatly enhance maintainability. The code is cleaner, more robust, and adheres to modern Python standards. I've found one minor issue regarding the use of ParamSpec for type hinting, which I've detailed in a specific comment. Overall, this is a high-quality contribution.

@lispandfound
Copy link
Contributor Author

@copilot Add test cases covering explicit raise statements in any module in the package that is not omitted in coverage in pyproject.toml.

Copy link

Copilot AI commented Nov 13, 2025

@lispandfound I've opened a new pull request, #359, to work on those changes. Once the pull request is ready, I'll request review from you.

Copilot AI and others added 3 commits November 13, 2025 14:27
* Initial plan

* Add test coverage for explicit raise statements

Co-authored-by: lispandfound <12835929+lispandfound@users.noreply.github.com>

* Add comprehensive test coverage for geo module

Co-authored-by: lispandfound <12835929+lispandfound@users.noreply.github.com>

---------

Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: lispandfound <12835929+lispandfound@users.noreply.github.com>
@lispandfound
Copy link
Contributor Author

@copilot Add basic tests for all the omitted modules listed in the pyproject.toml except the deprecated modules qcore/constants.py and qcore/formats.py.

Copy link

Copilot AI commented Nov 13, 2025

@lispandfound I've opened a new pull request, #360, to work on those changes. Once the pull request is ready, I'll request review from you.

joelridden
joelridden previously approved these changes Nov 28, 2025
@sungeunbae
Copy link
Member

Deprecation and removal strategy

I want to outline the deprecation and removal strategy for this PR to help understand why some functions got deleted and why some got deprecated.

  1. Any functions or modules only used in Pre-Processing or slurm_gm_workflow were removed. I will create a tag old on the master branch that will allow @sungeunbae to revert to that commit if he needs to re-run old workflow code.
  2. Any functions that had previously been marked in the docstring as deprecated have been explicitly upgraded to @deprecated code that will raise a warning (but still run) when invoked.
  3. Any code only used in Pre-Processing, slurm_gm_workflow and cybershake_investigation has been deprecated pending removal once Morteza has completed the Cybershake paper.

All remaining functions have been typed, tested, and documented. I will further pursue a strategy of eliminating code by downstreaming a couple of functions that are used only in one other repo.

Test refactor

Tests have been moved into the top-level tests folder, which is consistent with the other repositories. Tests are now run via github actions and the Jenkins integration has been removed. Test coverage is now close to 100% and is enforced for future changes to the repository. The eventual goal is 95%+ coverage for all dependencies of workflow code (I think this is the last repo without substantial testing).

Type-checking

Type-checking is now globally enabled. This is second-to-last repo I intend to add it to at this stage (IM_calculation is next). This is a further sanity check for all code in qcore. Several functions have @overload decorators to help the type-checker infer the return type for functions that have return_* keyword arguments. Ultimately I think we should probably refactor such functions to not have multiple return types because they make the code much harder to test, but for now I am focusing on providing better experiences for auto-completion, LLM usage, and confidence in code correctness.

All sensible, I reckon.

@sungeunbae
Copy link
Member

How can we efficiently review with this limitation?
image

@lispandfound
Copy link
Contributor Author

lispandfound commented Jan 6, 2026

After talking with @sungeunbae I am going to split this into two PRs:

  1. Removing all the functions,
  2. Adding type checking.

This should make it easier to review.

Actually maybe three:

  1. Remove all the functions,
  2. Add type checking,
  3. Add tests.

@lispandfound
Copy link
Contributor Author

Closed in favour of the broken up strategy.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants