Skip to content
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

Major code refactoring #104

Merged
merged 451 commits into from
Jun 25, 2024
Merged

Major code refactoring #104

merged 451 commits into from
Jun 25, 2024

Conversation

Olender
Copy link
Collaborator

@Olender Olender commented Jun 18, 2024

Major Code Refactoring

Overview

This PR introduces a comprehensive refactoring of the codebase to improve readability, maintainability, and performance. The changes encompass a wide range of modifications, including structural reorganization, code cleanup, and accuracy.

Key Changes

  1. Modularization:

    • Split large functions into smaller, more manageable classes (when appropriate) and methods. For example the old forward method is inside the new AcousticWave class. Its time propagation aspect was factored into the central_difference method, the form definition and solver construction into the relevant acoustic_solver_construction methods.
    • Encapsulated repeated code segments into reusable methods.
  2. Naming Conventions:

    • Renamed variables, functions, and classes to adhere to naming conventions and improve clarity. For instance, the dictionary input parameters have improved clarity.
    • The old dictionary input is still compatible. However, all dictionary inputs are now checked and sanity tested inside the Model_parameters class.
  3. Documentation:

    • Updated the README.md file to reflect the new structure and usage instructions.
    • Added docstrings to major functions and classes for better documentation.
    • Created various new jupyter notebook tutorials.
  4. Testing:

    • Refactored existing tests to match the new structure.
    • Added new tests to cover more cases and newly introduced functions.
    • Added tests that verify second order convergence in regards to time-stepping when compared with an analytical solution.
    • Added more robust tests in general (i.e. PML absorption, second order in time Ricker source propagation, working MMS tests, random direction gradient tests).
  5. Code Cleanup:

    • Removed unused variables and functions.
    • Updated outdated comments and removed redundant ones.
    • Made complex classes such as the sources and the cells-per-wavelength calculator infinitely more readable and usable.
  6. Overall reduced error

    • Removed wrong number of time-steps calculation.
    • Fixed code to obtain correct time convergence.
    • Gradient calculation without the PML now follows second order finite difference expected error when decreasing step size, and gives less error when compared with previous code.
  7. New functionality

    • Easier integration with SeismicMesh using the automatic meshing class.
    • Added easily modified examples of frequently used experiments for ease of testing new functionalities (i.e. Camembert model)
    • New utils functions that give us things such as an analytical solution to a Ricker propagation.

Impact

  • Maintainability: Codebase is now easier to navigate, understand, and maintain.
  • Accuracy: Overall better accuracy.
  • Readability: Enhanced readability due to consistent formatting, naming conventions, and comprehensive documentation.
  • Scalability: Modular code structure paves the way for easier scalability and addition of new features.

Testing

  • All tests have been run. However, it has been brought to my attention by @SouzaEM that the gradient test might fail in some specific random direction. I still haven been able to reproduce this error and plan to open an issue and add a fix for it later.
  • New tests have been added.
  • Manual testing has been performed to ensure functionality remains intact. This includes FWI with ROL. However, the current CI only includes tests for FWI with scipy (the ROL tests are in the code but are skipped when not installed). This is due to our runner not having pyROL installed (and pyROL is no longer maintained). We have been having problems with our local runner, and for the time being we can't install ROL in it.

Related Issues

  • #IssueNumber 94, 95, and 99.

Notes

  • Feedback and suggestions for further improvement are welcome.

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.

1 participant