Skip to content

[WIP] feat(edm): integrate EDM prediction framework into FIMS#1620

Open
aman-raj-srivastva wants to merge 39 commits into
NOAA-FIMS:main-edmfrom
aman-raj-srivastva:feature/edm-fims-integration
Open

[WIP] feat(edm): integrate EDM prediction framework into FIMS#1620
aman-raj-srivastva wants to merge 39 commits into
NOAA-FIMS:main-edmfrom
aman-raj-srivastva:feature/edm-fims-integration

Conversation

@aman-raj-srivastva

@aman-raj-srivastva aman-raj-srivastva commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

Summary

This PR integrates the EDM prediction framework into the FIMS statistical modeling pipeline.

Building on the standalone delay embedding infrastructure (#1528) and prediction framework (#1561), this PR incrementally connects EDM prediction algorithms to the FIMS objective function so that their predictions can participate in the joint negative log-likelihood alongside existing FIMS model components.

Target Branch: main-edm


Current Progress

This PR currently introduces the EDMModel<Type> module.

Implemented:

  • Added EDMModel<Type> as a new FIMS module
  • EDMModel<Type> inherits from FIMSObject<Type>
  • Updated edm.hpp to expose the new module

Planned Work

The following components will be added in subsequent commits:

  • Register EDMModel in Information<Type>
  • Integrate EDM evaluation into Model::Evaluate()
  • Complete the Rcpp add_to_fims_tmb_internal() interfaces
  • Expose prediction outputs through finalize()
  • Add end-to-end integration tests
  • Update documentation

Related


Instructions for code reviewer

👋Hello reviewer👋, thank you for taking the time to review this PR!

  • Please use this checklist during your review, checking off items that you have verified are complete but feel free to skip over items that are not relevant!
  • See the GitHub documentation for how to comment on a PR to indicate where you have questions or changes are needed before approving the PR.
  • Please use standard conventional messages for both commit messages and comments
  • PR reviews are a great way to learn so feel free to share your tips and tricks. However, when suggesting changes to the PR that are optional please include nit: (for nitpicking) as the comment type. For example, nit: I prefer using a data.frame() instead of a matrix because ...
  • Engage with the developer. Make it clear when the PR is approved by selecting the approved status, and potentially commenting on the PR with something like This PR is now ready to be merged.

Checklist

  • The code is well-designed
  • The code is designed well for both users and developers
  • Code coverage remains high- [ ] Comments are clear, useful, and explain why instead of what
  • Code is appropriately documented (doxygen and roxygen)

Aman and others added 30 commits June 9, 2026 21:59
added the comments to the following files:
	modified:   R/distribution_formulas.R
	modified:   R/fimsfit.R
	modified:   R/initialize_modules.R
	modified:   inst/include/common/information.hpp
	modified:   inst/include/common/model.hpp
	modified:   inst/include/distributions/functors/density_components_base.hpp
	modified:   inst/include/distributions/functors/normal_lpdf.hpp
	modified:   inst/include/interface/rcpp/rcpp_objects/rcpp_distribution.hpp
	modified:   src/FIMS.cpp
- Created rcpp_edm.hpp defining EDMInterfaceBase and DelayEmbeddingInterface wrapper classes.

- Exposed the construct(), construct_drop_missing(), and at() methods to R.

- Exposed fields/metadata (such as embedding_dimension, time_lag, n_rows, n_cols, values, and target_indices) to R.

- Registered the DelayEmbedding class in the RCPP_MODULE(fims) block within fims_modules.hpp.
- Add edm_embeddings list slot to the FIMSFrame S4 class to store
  delay-embedding results produced by create_edm_embedding()
- Add create_edm_embedding() helper that pulls a named time series
  from the data slot, calls the Rcpp DelayEmbedding module, and
  returns a new FIMSFrame with the result stored by name
- Add get_edm_embeddings() accessor following the existing get_*()
  pattern in fimsframe.R
- Add model_edm_matrix() accessor that returns a numeric matrix
  (n_rows x E) ready to pass to an EDM prediction module, following
  the existing model_*() pattern
- Register 'edm' as a valid FIMSFrame input type in
  data-raw/fims_input_types.R and regenerate data/fims_input_types.rda
- Add tests/testthat/test-edm-fimsframe.R with 6 test blocks covering
  default empty slot, error handling, input validation, metadata
  correctness, matrix shape, and chaining multiple embeddings

This completes proposal deliverable: 'Create structures in FIMSFrame
for embedded matrices' (Weeks 1-3).
…rget

Per mentor feedback (nathanvaughan-NOAA, stevemunch):

1. Selectable weighting kernel (SMapKernel enum):
   - kExponential (default): w = exp(-theta * d / d_mean)  [rEDM / Sugihara 1994]
   - kGaussian: w = exp(-theta^2 * (d/D)^2)               [Esguerra & Munch 2024]
   Both reduce to global OLS when theta=0. Kernel field exposed on
   SMapProjection<Type> so users can compare formulations.

2. One-step-ahead target alignment in MakeDelayEmbedding:
   Added forecast_horizon parameter (default=1). target_values[row] now
   points to series[target_index + h], matching the standard EDM
   formulation: predict x_{t+h} from the embedding state x_t.
   n_rows reduced by forecast_horizon to keep all target pointers in bounds.
   MakeDelayEmbeddingDropMissing forwards the parameter unchanged.

3. Tests: 12 S-Map + 9 Simplex = 21 passing GoogleTests.
- edm.hpp: remove stale 'S-map will be added' note; SMapProjection
  is already included
- delay_embedding.hpp: correct target_values struct description from
  'x_t' to 'x_{t+h}'; fix target_uncertainty comment to say
  sigma_{target_index + forecast_horizon} not sigma_t
@aman-raj-srivastva
aman-raj-srivastva force-pushed the feature/edm-fims-integration branch from fa181ff to 27e89b5 Compare July 9, 2026 20:18
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.

2 participants