Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR separates model selection functionality by introducing a new log‐likelihood based information criterion function and refactoring the online GAMLSS estimator to support different model selection approaches. Key changes include:
- Adding a new function, information_criterion_log_likelihood, to compute criteria based on log‐likelihood.
- Refactoring model selection and beta update functions in online_gamlss.py with new parameters and renaming of iteration variables.
- Updating convergence condition checks and internal data structures to support the new model selection logic.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/rolch/information_criteria.py | Adds a log-likelihood based IC function to support additional criteria. |
| src/rolch/estimators/online_gamlss.py | Refactors model selection methods and updates convergence conditions. |
Comments suppressed due to low confidence (1)
src/rolch/estimators/online_gamlss.py:823
- The updated convergence condition in the inner iteration now only checks 'it_inner > 1', which is not equivalent to the former cumulative condition (iteration_inner + iteration_outer >= 2). Please review this logic change to ensure it meets the intended convergence criteria.
if (abs(olddv - dv) <= self.abs_tol_inner) & (it_inner > 1):
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the information criterion functionality by replacing the old function‐based implementation with a new InformationCriterion class and updates related references across the codebase. Key changes include:
- Converting the information criterion calculation from a function to a class with methods from_rss and from_ll.
- Updating dependent modules (online_linear_model, init.py, and docs) to use the new class interface.
- Removing the previous select_best_model_by_information_criterion function.
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/rolch/information_criteria.py | Replaces function with class implementation and updates docs. |
| src/rolch/estimators/online_linear_model.py | Adapts model selection to use the new InformationCriterion class. |
| src/rolch/init.py | Removes legacy function references and updates exports. |
| docs/model_selection.md | Updates documentation reference to the new class. |
Comments suppressed due to low confidence (2)
src/rolch/information_criteria.py:6
- Typographical error: 'Calcuate' should be corrected to 'Calculate'.
"""Calcuate the information criteria.
src/rolch/information_criteria.py:14
- The documentation table includes the 'max' criterion, but the init method does not accept 'max'. Please update the documentation or adjust the allowed criteria to resolve this inconsistency.
| "max" | Select the largest model | |
|
I did the following changes to this PR:
Now we use |
What about EBIC and EFIC, see here: https://ieeexplore.ieee.org/abstract/document/10146465 |
There was a problem hiding this comment.
Pull Request Overview
This pull request refactors the model selection functionality by replacing the standalone functions with a dedicated InformationCriterion class.
- Introduces the InformationCriterion class with methods for calculating criteria from RSS and log-likelihood values.
- Updates references in estimator modules and documentation to use the new class.
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/rolch/information_criteria.py | Refactored information criterion functions into a class with methods from_rss and from_ll. |
| src/rolch/estimators/online_linear_model.py | Updated model selection to use the InformationCriterion class instead of the removed functions. |
| src/rolch/init.py | Removed deprecated function exports and exposes the new InformationCriterion class. |
| docs/model_selection.md | Updated API documentation to reflect the change to the InformationCriterion class. |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
This is quite well now.