Skip to content

Add PersistencePredictor baseline model - #677

Open
GiGiKoneti wants to merge 11 commits into
mllam:mainfrom
GiGiKoneti:feat/persistence-predictor
Open

Add PersistencePredictor baseline model#677
GiGiKoneti wants to merge 11 commits into
mllam:mainfrom
GiGiKoneti:feat/persistence-predictor

Conversation

@GiGiKoneti

Copy link
Copy Markdown
Contributor

Describe your changes

  • Add PersistencePredictor, a baseline StepPredictor model that returns the previous state unchanged at each autoregressive step.
  • Register PersistencePredictor as persistence in the MODELS registry inside neural_lam/models/__init__.py.
  • Add validation logic in neural_lam/train_model.py to prevent training the persistence model, raising a ValueError if training is attempted.
  • Add comprehensive unit tests covering the forward pass, kwargs absorption, forecaster unrolling, predicts_std behavior, and training error prevention.

No changes to any existing prediction pathways.

None.

Issue Link

closes #676

Type of change

  • 🐛 Bug fix (non-breaking change that fixes an issue)
  • ✨ New feature (non-breaking change that adds functionality)
  • 💥 Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • 📖 Documentation (Addition or improvements to documentation)

Checklist before requesting a review

  • My branch is up-to-date with the target branch - if not update your fork with the changes from the target branch (use pull with --rebase option if possible).
  • I have performed a self-review of my code
  • For any new/modified functions/classes I have added docstrings that clearly describe its purpose, expected inputs and returned values
  • I have placed in-line comments to clarify the intent of any hard-to-understand passages of my code
  • I have updated the README to cover introduced code changes
  • I have added tests that prove my fix is effective or that my feature works
  • I have given the PR a name that clearly describes the change, written in imperative form (context).
  • I have requested a reviewer and an assignee (assignee is responsible for merging). This applies only if you have write access to the repo, otherwise feel free to tag a maintainer to add a reviewer and assignee.

Checklist for reviewers

Each PR comes with its own improvements and flaws. The reviewer should check the following:

  • the code is readable
  • the code is well tested
  • the code is documented (including return types and parameters)
  • the code is easy to maintain

Author checklist after completed review

  • I have added a line to the CHANGELOG describing this change, in a section
    reflecting type of change (add section where missing):
    • added: when you have added new functionality
    • changed: when default behaviour of the code has been changed
    • fixes: when your contribution fixes a bug
    • maintenance: when your contribution is relates to repo maintenance, e.g. CI/CD or documentation

Checklist for assignee

  • PR is up to date with the base branch
  • the tests pass
  • (if the PR is not just maintenance/bugfix) the PR is assigned to the next milestone. If it is not, propose it for a future milestone.
  • author has added an entry to the changelog (and designated the change as added, changed, fixed or maintenance)
  • Once the PR is ready to be merged, squash commits and merge the PR.

@sadamov sadamov left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks @GiGiKoneti, this is clean and well-scoped. I ran it locally: the 5 tests pass, pre-commit is clean, and --eval test --model persistence runs end-to-end with the loss growing by lead time...

A few small asks inline, none of them block the logic.

  • the training guard in train_model.py compares against the literal "persistence". Fine for a single baseline, though a trainable = False class attribute would be more robust if more analytical baselines arrive later.
  • --output_std is silently ignored with persistence (documented, harmless). A logger.warning would make the no-op explicit.
  • README is still unchecked in the PR checklist. A one-liner noting --model persistence needs --eval val/test would help discoverability.

Comment thread CHANGELOG.md Outdated
Comment thread CHANGELOG.md Outdated
Comment thread tests/test_persistence.py Outdated
@GiGiKoneti

Copy link
Copy Markdown
Contributor Author

@sadamov

  • Replaced the hardcoded training guard with a trainable class attribute on StepPredictor.
  • Fixed the cascading test_eval_without_load_warning failure caused by the new MODELS dictionary lookup in train_model.py.
  • Addressed all inline suggestions for the changelog, test assertions, std warning, and README.

@sadamov
sadamov requested a review from joeloskarsson June 22, 2026 18:40
@sadamov sadamov added the enhancement New feature or request label Jun 22, 2026
@sadamov sadamov added this to the v0.8.0 (proposed) milestone Jun 22, 2026
Comment thread neural_lam/train_model.py Outdated
Comment thread neural_lam/train_model.py Outdated
@joeloskarsson

Copy link
Copy Markdown
Collaborator

Looking very good, I added some ideas above that could be small improvements :)

@joeloskarsson joeloskarsson left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Nice! LGTM

@sadamov sadamov left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Clean and correct, already approved. A few small polish asks inline, none block the logic.

Comment thread tests/test_train_model_warnings.py Outdated
Comment thread tests/test_persistence.py
@GiGiKoneti

GiGiKoneti commented Jun 29, 2026

Copy link
Copy Markdown
Contributor Author

Done!

  • Removed the vestigial patches from test_eval_without_load_warning in tests/test_train_model_warnings.py as suggested.
  • Added the missing predictor.trainable is False assertion to test_persistence_predictor_returns_prev_state in tests/test_persistence.py to exercise the dynamic trainable property.

@sadamov
sadamov self-requested a review June 30, 2026 07:32
@sadamov sadamov added the ready Review complete - proposed for milestone label Jul 12, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request ready Review complete - proposed for milestone

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add baseline persistence model forecaster

3 participants