Skip to content

Conversation

@ma595
Copy link
Member

@ma595 ma595 commented Nov 18, 2025

The unused code removal can be separated into three distinct categories.

Unused optimization code

  • Remove src/nemo_spinup_forecast/optimization.py

Deletions relating to restart (relevant parts will be moved to another repo)

  • src/nemo_spinup_forecast/restart.py
  • restart_toolbox.sh
  • standalone/restarter.py
  • standalone/restarter_V2.py

Unused tools

  • CMIP_to_xarray.sh
  • xarray_to_CMIP.sh
  • rename.sh

Fixes #79 #80

Copilot AI review requested due to automatic review settings November 18, 2025 17:47
@ma595 ma595 changed the base branch from main to pyproject November 18, 2025 17:48
@ma595 ma595 changed the title Remove unused code in the forecasting step Remove unused code in forecasting pipeline Nov 18, 2025
Copy link

Copilot AI left a 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 pull request restructures the package from a lib/ directory-based approach to a proper Python package structure under src/nemo_spinup_forecast/, while removing unused code and updating configuration files for modern Python packaging practices.

Key Changes:

  • Migrated code from lib/ to src/nemo_spinup_forecast/ package structure
  • Removed unused utility scripts (shell scripts in tools/) and standalone Python files
  • Replaced setup.py + requirements.txt + pytest.ini with modern pyproject.toml
  • Added new test infrastructure (download_test_data.sh) and expanded test coverage

Reviewed Changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated no comments.

Show a summary per file
File Description
tools/xarray_to_CMIP.sh Deleted unused shell script for coordinate renaming
tools/rename.sh Deleted unused shell script for directory reorganization
tools/CMIP_to_xarray.sh Deleted unused shell script for coordinate transformation
standalone/restarter_V2.py Deleted unused standalone restart file manipulation script
standalone/restarter.py Deleted unused standalone restart file manipulation script
lib/restart.py Deleted old library file (functionality not migrated to new package)
lib/optimization.py Deleted unused optimization utilities
setup.py Deleted legacy setup file in favor of pyproject.toml
requirements.txt Deleted in favor of pyproject.toml dependencies
pytest.ini Deleted in favor of pyproject.toml test configuration
main_restart.py Deleted unused restart processing script
pyproject.toml Added modern Python package configuration with build system, dependencies, and dev tools
tests/download_test_data.sh Added script to download test data from GitHub releases
tests/test_forecast.py Expanded with new NetCDF file creation and testing functions
tests/*.py Updated imports from lib.* to src.nemo_spinup_forecast.*
src/nemo_spinup_forecast/forecast_method.py New file containing forecaster base classes and implementations
src/nemo_spinup_forecast/dimensionality_reduction.py New file with PCA and Kernel PCA implementations
src/nemo_spinup_forecast/density.py Migrated from lib/, cleaned up numpy imports and added grav constant
src/nemo_spinup_forecast/utils.py Updated import path for Simulation class
src/nemo_spinup_forecast/forecast.py Removed unnecessary noqa comment
main.py Updated imports to use new package structure
ruff.toml Updated file exclusions to match new structure

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@ma595 ma595 self-assigned this Nov 18, 2025
@ma595 ma595 added health help wanted Extra attention is needed labels Nov 18, 2025
@ma595
Copy link
Member Author

ma595 commented Nov 18, 2025

We want the forecast library to consist of code relating to the forecast step only. We intend to create a separate restart repo containing the necessary steps for modifying a pre-existing restart based on the ML hop or forecast.
@GuillaumeGACHON can you check if the remaining deletions make sense?

@ma595 ma595 requested a review from isaacaka November 18, 2025 17:56
@ma595 ma595 added the cleanup Clean up the repo label Nov 18, 2025
@GuillaumeGACHON
Copy link

Everything seems good to me !

@ma595 ma595 force-pushed the rm-unused branch 2 times, most recently from a2b1201 to 46bbd0a Compare November 25, 2025 18:34
@ma595 ma595 changed the base branch from pyproject to main November 25, 2025 21:17
Copy link
Collaborator

@isaacaka isaacaka left a comment

Choose a reason for hiding this comment

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

Also remove Notebooks/Restart.ipynb

- restart code and scripts
- optimization step
- tools directory
- restart notebook

These scripts/tools sit outside the forecasting pipeline.
@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

Copy link
Collaborator

@isaacaka isaacaka left a comment

Choose a reason for hiding this comment

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

Good to merge

@ma595 ma595 merged commit 25a7051 into main Nov 26, 2025
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cleanup Clean up the repo health help wanted Extra attention is needed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Remove restart code

4 participants