-
Notifications
You must be signed in to change notification settings - Fork 8
Fold cc factory into modeling tools #608
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
Consolidate the ccl_factory module into modeling_tools to improve code organization. The CCL factory functionality is closely related to the modeling tools and belongs in the same module. Changes: - Move ccl_factory submodules into modeling_tools as _ccl_* files - Update modeling_tools __init__ to export CCL factory classes - Replace ccl_factory with deprecation wrapper that re-exports - Update all imports across examples, tests, and connectors - Add deprecation warning when ccl_factory is imported - Add tests for deprecated module backward compatibility - Rename test_ccl_factory.py to test_modeling_tools_ccl_factory.py
Correct the module path reference in the two-point integration tutorial from `firecrown.generators.inferred_galaxy_zdist` to `firecrown.generators`, which is the accurate import location for the lens and source redshift bin collections.
The firecrown.parameters module has been moved into firecrown.updatable to better reflect the architectural relationship between parameters and updatable objects. All parameter-related classes (DerivedParameter, DerivedParameterCollection, InternalParameter, ParamsMap, RequiredParameters, SamplerParameter) and functions (handle_unused_params, parameter_get_full_name, register_new_updatable_parameter) are now exported from firecrown.updatable. The old firecrown.parameters module remains as a deprecated compatibility layer that re-exports from firecrown.updatable with deprecation warnings. All imports throughout the codebase have been updated to use firecrown.updatable instead of firecrown.parameters. Tests have been added to verify the deprecated module continues to work correctly with appropriate warnings, and existing parameter tests have been renamed to reflect their new location.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR consolidates code organization by moving the ccl_factory module functionality into modeling_tools and the parameters module functionality into updatable. This refactoring better reflects the architectural relationships between these components while maintaining backward compatibility through deprecated compatibility layers that re-export from the new locations with deprecation warnings.
Changes:
- Moved CCL factory classes from
firecrown.ccl_factorytofirecrown.modeling_tools - Moved parameter-related classes from
firecrown.parameterstofirecrown.updatable - Updated all imports throughout the codebase (examples, tests, connectors, and tutorials)
Reviewed changes
Copilot reviewed 92 out of 96 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
| firecrown/ccl_factory/init.py | Deprecated compatibility layer re-exporting from modeling_tools |
| firecrown/parameters/init.py | Deprecated compatibility layer re-exporting from updatable |
| firecrown/modeling_tools/init.py | Expanded to include CCL factory exports |
| firecrown/modeling_tools/ccl*.py | Moved CCL factory implementation files |
| firecrown/updatable/init.py | Expanded to include parameter class exports |
| firecrown/updatable/parameters*.py | Moved parameter implementation files |
| tests/test_*_deprecated.py | New tests for backward compatibility |
| tutorial/*.qmd | Updated imports to new module paths |
| examples/**/*.py | Updated imports to new module paths |
| tests/**/*.py | Updated imports to new module paths (except for backward compat testing) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| match="firecrown.ccl_factory is deprecated and will be removed", | ||
| ): | ||
| # pylint: disable=import-outside-toplevel,unused-import | ||
| import firecrown.ccl_factory # noqa: F401 |
Copilot
AI
Jan 16, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Module 'firecrown.ccl_factory' is imported with both 'import' and 'import from'.
| We just verify the module is importable. | ||
| """ | ||
| # pylint: disable=import-outside-toplevel,unused-import | ||
| import firecrown.ccl_factory # noqa: F401 |
Copilot
AI
Jan 16, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Module 'firecrown.ccl_factory' is imported with both 'import' and 'import from'.
| def test_module_import_as_alias(): | ||
| """Test importing the module with an alias.""" | ||
| # pylint: disable=import-outside-toplevel | ||
| import firecrown.ccl_factory as fac |
Copilot
AI
Jan 16, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Module 'firecrown.ccl_factory' is imported with both 'import' and 'import from'.
| def test_ccl_factory_all_list(): | ||
| """Test that __all__ is preserved in deprecated module.""" | ||
| # pylint: disable=import-outside-toplevel | ||
| import firecrown.ccl_factory |
Copilot
AI
Jan 16, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Module 'firecrown.ccl_factory' is imported with both 'import' and 'import from'.
| import sacc | ||
|
|
||
| import firecrown.parameters | ||
| import firecrown.updatable |
Copilot
AI
Jan 16, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Module 'firecrown.updatable' is imported with both 'import' and 'import from'.
| warnings.simplefilter("always") | ||
| # Import should trigger warning | ||
| # pylint: disable=unused-import,import-outside-toplevel | ||
| import firecrown.parameters # noqa: F401 |
Copilot
AI
Jan 16, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Module 'firecrown.parameters' is imported with both 'import' and 'import from'.
| def test_parameters_exports_all_names(): | ||
| """All expected names should be available from firecrown.parameters.""" | ||
| # pylint: disable=import-outside-toplevel | ||
| import firecrown.parameters as params |
Copilot
AI
Jan 16, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Module 'firecrown.parameters' is imported with both 'import' and 'import from'.
| def test_parameters_exports_are_same_as_updatable(): | ||
| """Exported names should be the same objects as in firecrown.updatable.""" | ||
| # pylint: disable=import-outside-toplevel | ||
| import firecrown.parameters as params |
Copilot
AI
Jan 16, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Module 'firecrown.parameters' is imported with both 'import' and 'import from'.
| from firecrown.ccl_factory import CCLFactory, PoweSpecAmplitudeParameter | ||
| import firecrown.parameters as fcp | ||
| from firecrown.modeling_tools import CCLFactory, PoweSpecAmplitudeParameter | ||
| import firecrown.updatable as fcp |
Copilot
AI
Jan 16, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Module 'firecrown.updatable' is imported with both 'import' and 'import from'.
| import sacc | ||
|
|
||
| import firecrown.parameters | ||
| import firecrown.updatable |
Copilot
AI
Jan 16, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Module 'firecrown.updatable' is imported with both 'import' and 'import from'.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #608 +/- ##
=======================================
Coverage 100.0% 100.0%
=======================================
Files 151 151
Lines 9035 9044 +9
Branches 1033 1033
=======================================
+ Hits 9035 9044 +9
🚀 New features to boost your workflow:
|
Description
This PR consolidates the
ccl_factorymodule intomodeling_toolsand moves theparametersmodule intoupdatableto improve code organization and better reflect the architectural relationships between these components.The CCL factory functionality is closely related to modeling tools and now resides within the same module. Similarly, parameter-related classes are now part of the
updatablemodule, reflecting their tight coupling with updatable objects.Both old modules (
firecrown.ccl_factoryandfirecrown.parameters) remain as deprecated compatibility layers that re-export from their new locations with deprecation warnings, ensuring backward compatibility for existing code.All imports throughout the codebase (examples, tests, connectors, and tutorials) have been updated to use the new module paths.
Type of Change
Checklist:
The following checklist will make sure that you are following the code style and
guidelines of the project as described in the
contributing page.
bash pre-commit-checkand fixed any issues