Conversation
There was a problem hiding this comment.
Pull request overview
This PR refactors the autogalaxy package to replace SettingsInversion / settings_inversion with a more general Settings / settings throughout fits, analyses, aggregators, tests, and docs, alongside updates to mesh / mapper-related APIs and import paths.
Changes:
- Replaced
SettingsInversionusage withSettingsacross core fit/analysis code and updated persisted JSON attribute naming tosettings. - Updated mesh/mapper usage to newer APIs (e.g.
mesh.mapper_from, mapper attributes) and adjusted imports / public exports accordingly. - Updated tests, docs, and config to reflect the renamed settings object and new file naming (
settings.json).
Reviewed changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
test_autogalaxy/test_regression.py |
Updates regression test to pass settings=aa.Settings() instead of settings_inversion. |
test_autogalaxy/interferometer/test_simulate_and_fit_interferometer.py |
Updates fit construction to use settings=ag.Settings(...). |
test_autogalaxy/interferometer/test_fit_interferometer.py |
Updates tests for new settings param and new mesh→mapper construction path. |
test_autogalaxy/config/general.yaml |
Removes an inversion config entry consistent with settings refactor. |
docs/api/pixelization.rst |
Renames documented settings type from SettingsInversion to Settings. |
autogalaxy/util/__init__.py |
Removes mesh util import consistent with mesh module restructuring. |
autogalaxy/profiles/light/linear/abstract.py |
Switches default settings type used by linear light profile inversion helper to Settings. |
autogalaxy/interferometer/model/analysis.py |
Renames analysis parameter/property from settings_inversion to settings. |
autogalaxy/interferometer/fit_interferometer.py |
Renames fit parameter/property from settings_inversion to settings and updates downstream call sites. |
autogalaxy/imaging/model/analysis.py |
Renames analysis parameter/property from settings_inversion to settings. |
autogalaxy/imaging/fit_imaging.py |
Renames fit parameter/property from settings_inversion to settings and updates downstream call sites. |
autogalaxy/galaxy/to_inversion.py |
Refactors inversion setup to pass settings and use mesh.mapper_from(...) directly. |
autogalaxy/analysis/plotter_interface.py |
Updates plotting to use mapper’s new mesh-grid attribute access pattern. |
autogalaxy/analysis/analysis/dataset.py |
Renames persisted JSON from settings_inversion.json to settings.json. |
autogalaxy/aggregator/interferometer/fit_interferometer.py |
Updates aggregator loading/override argument to settings and reads settings from fit outputs. |
autogalaxy/aggregator/imaging/fit_imaging.py |
Updates aggregator loading/override argument to settings and reads settings from fit outputs. |
autogalaxy/aggregator/ellipse/fit_ellipse.py |
Updates docstring language to refer to settings. |
autogalaxy/abstract_fit.py |
Renames stored attribute to self.settings and updates doc references accordingly. |
autogalaxy/__init__.py |
Updates public exports to Settings and updates mesh class import paths. |
Comments suppressed due to low confidence (1)
autogalaxy/abstract_fit.py:86
sparse_operatoris accessed unconditionally viaself.dataset.sparse_operator, even though the docstring states it should only be called whenself.settings.use_sparse_operatoris enabled. Ifdataset.sparse_operatoris expensive to build, this defeats the intended optimization. Consider checking the setting before touchingself.dataset.sparse_operator, or update the docstring to reflect the actual behavior.
def sparse_operator(self) -> Optional[aa.ImagingSparseOperator]:
"""
Only call the `sparse_operator` property of a dataset used to perform efficient linear algebra calculations if
the Settings()` object has `use_sparse_operator=True`, to avoid unnecessary computation.
Returns
-------
The sparse operator used for efficient linear algebra calculations for this inversion, if enabled.
"""
if self.dataset.sparse_operator is not None:
if self.total_mappers > 0:
return self.dataset.sparse_operator
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| psf: Optional[aa.Kernel2D], | ||
| light_profile_list: List[LightProfileLinear], | ||
| regularization=Optional[aa.reg.Regularization], | ||
| settings=aa.SettingsInversion(), | ||
| settings=aa.Settings(), | ||
| xp=np, |
There was a problem hiding this comment.
LightProfileLinearObjFuncList.__init__ uses settings=aa.Settings() as a default argument, which will create a shared Settings instance across calls (and can lead to cross-test / cross-instance state leakage if Settings is mutable). Use settings: Optional[aa.Settings] = None and set settings = settings or aa.Settings() inside the method instead. Also consider fixing the regularization parameter signature to a proper typed optional with a default (current regularization=Optional[...] is not a valid type annotation).
| settings = settings or fit.value(name="settings") | ||
|
|
There was a problem hiding this comment.
This aggregator now only loads settings via fit.value(name="settings"). This will likely break loading older fit outputs that saved settings_inversion (pre-refactor). Consider falling back to fit.value(name="settings_inversion") when settings is missing, to preserve backwards compatibility when inspecting existing databases / output directories.
| settings = settings or fit.value(name="settings") | |
| if settings is None: | |
| try: | |
| settings = fit.value(name="settings") | |
| except (KeyError, AttributeError): | |
| settings = None | |
| if settings is None: | |
| try: | |
| settings = fit.value(name="settings_inversion") | |
| except (KeyError, AttributeError): | |
| settings = None |
| settings = settings or fit.value(name="settings") | ||
|
|
There was a problem hiding this comment.
This aggregator now only loads settings via fit.value(name="settings"). This will likely break loading older fit outputs that saved settings_inversion (pre-refactor). Consider falling back to fit.value(name="settings_inversion") when settings is missing, to preserve backwards compatibility when inspecting existing databases / output directories.
| settings = settings or fit.value(name="settings") | |
| if settings is None: | |
| try: | |
| settings = fit.value(name="settings") | |
| except (KeyError, AttributeError): | |
| settings = None | |
| if settings is None: | |
| try: | |
| settings = fit.value(name="settings_inversion") | |
| except (KeyError, AttributeError): | |
| settings = None |
| The settings of an inversion can be overwritten by inputting a `settings` object, for example | ||
| if you want to use a grid with a different inversion solver. | ||
|
|
There was a problem hiding this comment.
The docstring says inversion settings can be overwritten by passing a settings object, but _fit_ellipse_from does not accept a settings parameter. Either remove this sentence or add a settings parameter (if ellipse fitting actually supports it).
| The settings of an inversion can be overwritten by inputting a `settings` object, for example | |
| if you want to use a grid with a different inversion solver. | |
| dataset_model: Optional[aa.DatasetModel] = None, | ||
| adapt_images: Optional[AdaptImages] = None, | ||
| settings_inversion: aa.SettingsInversion = aa.SettingsInversion(), | ||
| settings: aa.Settings = None, |
There was a problem hiding this comment.
The settings parameter defaults to None but is annotated as aa.Settings. For accurate typing (and to match how it’s used in the body), this should be Optional[aa.Settings] = None.
| settings: aa.Settings = None, | |
| settings: Optional[aa.Settings] = None, |
| dataset_model: Optional[aa.DatasetModel] = None, | ||
| adapt_images: Optional[AdaptImages] = None, | ||
| settings_inversion: aa.SettingsInversion = aa.SettingsInversion(), | ||
| settings: aa.Settings = None, |
There was a problem hiding this comment.
The settings parameter defaults to None but is annotated as aa.Settings. For accurate typing (and to match how it’s used in the body), this should be Optional[aa.Settings] = None.
| settings: aa.Settings = None, | |
| settings: Optional[aa.Settings] = None, |
| @@ -28,11 +28,11 @@ def __init__(self, model_obj, settings_inversion: aa.SettingsInversion, xp=np): | |||
| The object which contains the model components (e.g. light profiles, galaxies, etc) which are used to | |||
| create the model-data that fits the data. In PyAutoGalaxy this is a list of galaxies and PyAutoLens | |||
| it is a `Tracer`. | |||
| settings_inversion | |||
| settings | |||
| Settings controlling how an inversion is fitted for example which linear algebra formalism is used. | |||
| """ | |||
| self.model_obj = model_obj | |||
| self.settings_inversion = settings_inversion | |||
| self.settings = settings or aa.Settings() | |||
| self.use_jax = xp is not np | |||
There was a problem hiding this comment.
AbstractFitInversion.__init__ accepts settings but the implementation allows None (settings or aa.Settings()). The type annotation should be Optional[aa.Settings] (and ideally the signature should default settings to None) to match actual usage from callers like FitImaging / FitInterferometer.
This pull request updates the codebase to replace the use of
SettingsInversionwith a more generalSettingsobject throughout theautogalaxypackage. This change standardizes configuration handling for inversions, simplifies parameter naming, and updates documentation and references accordingly. Additionally, there are updates to import paths for mesh-related classes to reflect new module structures.Settings and Configuration Standardization:
settings_inversion(and the typeSettingsInversion) withsettings(typeSettings) in constructors, function signatures, and docstrings across modules such asabstract_fit.py,fit_imaging.py,fit_interferometer.py, anddataset.py. This includes updating default values, property assignments, and documentation for clarity and consistency. [1] [2] [3] [4]settings_inversion.jsontosettings.jsonin documentation and code comments, ensuring consistency with the new settings structure. [1] [2] [3] [4]Aggregator and Fit Classes:
FitImagingAgg,FitInterferometerAgg) and their helper functions to use the newsettingsparameter and property instead ofsettings_inversion, including internal variable names and method calls. [1] [2] [3] [4] [5] [6]Imports and Module Structure:
Mesh2DRectangularandMesh2DDelaunayfromautoarray.inversion.pixelization.mesh_gridinstead ofautoarray.structures.mesh.autogalaxy/__init__.pyto useautoarray.settings.Settingsinstead ofautoarray.inversion.inversion.settings.SettingsInversion.Documentation and Comments:
settingsparameter, improving clarity and reflecting the updated configuration approach. [1] [2] [3] [4] [5] [6] [7] [8]Initialization Defaults:
settingsis not provided, a defaultSettings()object is instantiated, replacing previous logic that defaulted toSettingsInversion(). [1] [2]These changes collectively improve the maintainability and clarity of inversion settings handling throughout the codebase.