Ensmeble-matrix changes #122
Conversation
Merge branch 'main' of https://github.com/MathiasMNilsen/PET
There was a problem hiding this comment.
Pull request overview
This pull request implements changes to handle ensemble-matrix state representations and improve logging functionality across the codebase. The changes involve transitioning from dictionary-based state storage to matrix-based ensemble handling, introducing a new logger class, and adding comprehensive test coverage for the toggle_ml_state function.
Changes:
- Introduced new test file for
toggle_ml_statefunction with comprehensive test coverage - Refactored state handling from dictionary-based to matrix-based ensemble representation
- Added new
PetLoggerclass for improved logging with formatted table output - Updated multiple update schemes to use new ensemble matrix interface
Reviewed changes
Copilot reviewed 27 out of 27 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_toggle_ml_state.py | New test suite for toggle_ml_state function |
| popt/update_schemes/subroutines/subroutines.py | Added logging decorators for line search procedures |
| popt/update_schemes/linesearch.py | Updated to use new logger and state handling |
| popt/misc_tools/optim_tools.py | Simplified toggle_ml_state to work with matrices instead of dictionaries |
| popt/loop/optimize.py | Replaced standard logging with PetLogger |
| popt/loop/ensemble_generalized.py | Updated to use matrix-based state representation |
| popt/loop/ensemble_gaussian.py | Refactored gradient/hessian methods for matrix ensemble |
| popt/loop/ensemble_base.py | Core changes to support matrix-based ensemble handling |
| popt/cost_functions/npv.py | Removed blank line |
| pipt/update_schemes/update_methods_ns/* | Updated update methods to use new ensemble matrix interface |
| pipt/update_schemes/multilevel.py | Simplified multilevel handling with new structure |
| pipt/update_schemes/esmda.py | Updated to use matrix ensemble and new logger |
| pipt/update_schemes/es.py | Updated state handling |
| pipt/update_schemes/enrml.py | Updated to use matrix ensemble |
| pipt/update_schemes/enkf.py | Updated to use matrix ensemble |
| pipt/misc_tools/extract_tools.py | Added extract_initial_controls function and corrected spelling |
| pipt/misc_tools/ensemble_tools.py | New module with ensemble manipulation utilities |
| pipt/misc_tools/analysis_tools.py | Added truncSVD function and removed deprecated code |
| pipt/loop/ensemble.py | Major refactoring to use matrix-based ensemble |
| pipt/loop/assimilation.py | Updated to use new ensemble matrix interface |
| misc/ecl.py | Commented out logging statements |
| input_output/read_config.py | Added read function and improved error handling |
| ensemble/logger.py | New PetLogger class implementation |
| ensemble/ensemble.py | Updated to use matrix-based ensemble and new logger |
Comments suppressed due to low confidence (1)
popt/loop/ensemble_base.py:1
- Duplicate condition check:
isinstance(limits, dict)is checked twice. This appears to be a copy-paste error.
# External imports
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| self.logger(**{ | ||
| 'iter.': 0, | ||
| fun_xk_symbol: self.fk, | ||
| jac_inf_symbol: la.norm(self.jk, np.inf), | ||
| 'step-size': self.step_size | ||
| }) |
There was a problem hiding this comment.
The logger is being called with keyword arguments but fun_xk_symbol and jac_inf_symbol are undefined variables. These should be string literals or defined elsewhere in the code.
popt/update_schemes/linesearch.py
Outdated
| pk = - np.matmul(self.Hk_inv, self._jk) | ||
| if self.method == 'Newton-CG': | ||
| pk = newton_cg(self._jk, Hk=self._Hk, xk=self._xk, jac=self.jac, logger=self.logger.info) | ||
| pk = newton_cg(self.jk, Hk=self.Hk, xk=self.xk, jac=self._jac, logger=self.logger) |
There was a problem hiding this comment.
Inconsistent attribute naming: uses self.jk, self.Hk, self.xk but also self._jac. Either all should have underscore prefix or none should.
| pk = newton_cg(self.jk, Hk=self.Hk, xk=self.xk, jac=self._jac, logger=self.logger) | |
| pk = newton_cg(self.jk, Hk=self.Hk, xk=self.xk, jac=self.jac, logger=self.logger) |
popt/update_schemes/linesearch.py
Outdated
| if (self.step_size_adapt == 1) and (np.dot(pk, self.jk) != 0): | ||
| alpha = 2*(self.fk - self.f_old)/np.dot(pk, self.jk) | ||
| elif (self.step_size_adapt == 2) and (np.dot(pk, self.jk) != 0): |
There was a problem hiding this comment.
Line 543 uses self.jk but line 545 uses self._jk. This inconsistency suggests one might be incorrect.
| if (self.step_size_adapt == 1) and (np.dot(pk, self.jk) != 0): | |
| alpha = 2*(self.fk - self.f_old)/np.dot(pk, self.jk) | |
| elif (self.step_size_adapt == 2) and (np.dot(pk, self.jk) != 0): | |
| if (self.step_size_adapt == 1) and (np.dot(pk, self._jk) != 0): | |
| alpha = 2*(self.fk - self.f_old)/np.dot(pk, self._jk) | |
| elif (self.step_size_adapt == 2) and (np.dot(pk, self._jk) != 0): |
| from geostat.decomp import Cholesky | ||
|
|
||
|
|
||
| def matrix_to_dict(matrix: np.ndarray, indecies: dict[tuple]) -> dict: |
There was a problem hiding this comment.
Corrected spelling of 'indecies' to 'indices' (used consistently throughout the file).
| print(msg) | ||
| self.logger(msg) | ||
| if enX.shape[1] > 1: | ||
| enX[:, list_crash[index]] = deepcopy(self.enX[:, element]) |
There was a problem hiding this comment.
Potential AttributeError: self.enX may not exist at this point since it was set to None earlier (line 216). Should use the local enX variable instead.
| enX[:, list_crash[index]] = deepcopy(self.enX[:, element]) | |
| enX[:, list_crash[index]] = deepcopy(enX[:, element]) |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
3 participants
No description provided.