Skip to content

Add MCNNMPanelSolver fit method#31

Open
fedemolina wants to merge 1 commit into
TianyiPeng:mainfrom
fedemolina:feature/mcnnm-fit-method
Open

Add MCNNMPanelSolver fit method#31
fedemolina wants to merge 1 commit into
TianyiPeng:mainfrom
fedemolina:feature/mcnnm-fit-method

Conversation

@fedemolina

Copy link
Copy Markdown
Contributor

Summary

  • Add MCNNMPanelSolver.fit(...) as a convenience dispatcher for suggested rank, fixed regularizer, and cross-validation workflows.
  • Add focused dispatch tests for the new MC-NNM fit API.
  • Update panel tutorial notebooks to use the new API while documenting the older functional and explicit solver APIs.
  • Update the datasets tutorial notebook to use shared dataset loader utilities instead of duplicating loader code, with local checkout path handling.

Closes #19.

Why

The MC-NNM solver already exposed the underlying solve methods, but did not match the fit style used by the other panel solvers. The tutorials also needed to show the new workflow while still making the older API discoverable.

Validation

  • env PYTHONPATH=src poetry run pytest tests/test_mcnnm_fit.py
  • env PYTHONPATH=src poetry run pytest src/causaltensor/tests/test_synthetic_class.py::TestSyntheticClass::test_mc src/causaltensor/tests/test_real_class.py::TestRealClass::test_mc
  • Executed tutorials/datasets.ipynb, tutorials/Panel Data Example.ipynb, and tutorials/Panel_Data_Example.ipynb via nbclient from both repo root and tutorials/ working directories, without PYTHONPATH.
  • git diff --check

@TianyiPeng TianyiPeng left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Clean, focused PR. The dispatch pattern in fit() is well-designed and the tests are thorough.

What works well

  • Clear dispatch logic: suggest_r > l > cross-validation — intuitive and documented in the docstring.
  • Smart test strategy: monkeypatching the underlying solve methods verifies dispatch routing without running expensive solvers. Fast and isolated.
  • All dispatch paths covered: suggest_r priority, l priority, CV default (K=2), CV with custom K, and missing O error.
  • Newline at EOF fixed for MCNNM.py.

Issues

1. Test file placement is inconsistent

The new test is at tests/test_mcnnm_fit.py (repo root), but existing tests live under src/causaltensor/tests/. Either move the new test to match the existing location, or note the intentional separation (e.g., unit tests vs integration tests). As-is, poetry run pytest from root may or may not find both locations depending on pytest config.

2. No return type in fit() docstring

The method returns a Result (from whichever underlying solver is dispatched), but neither the docstring nor type annotations mention the return value. Adding Returns: Result would help discoverability, especially since fit() is meant to be the primary user-facing entry point.

3. Tutorial notebook diffs are large (+4k/-2k lines) and hard to review from patches

The PR description says they were executed via nbclient — I'll trust that. But notebook output cells in the diff make it hard to spot code changes. Consider clearing outputs before committing if the reviewers need to focus on the code changes, and re-running in CI.

Smaller notes

  • fit() raises ValueError("O must be provided.") when O is None — good. But other MCNNM methods (e.g., solve_with_regularizer) also take O and silently use it. Worth asking whether O should be passed at construction time instead (like SDIDPanelSolver(Z, O)) for API consistency, though that's a bigger design discussion outside this PR's scope.
  • The fit() dispatch means passing both suggest_r and l silently ignores l. The docstring says "dispatch order is suggest_r > l > CV" which is correct, but consider logging a warning if both are passed — easy mistake to make.

Overall this is ready with the test-placement question resolved. Nice work.


Generated by Claude Code

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.

MCNNMPanelSolver fit method

2 participants