Skip to content

Packaging improvements#92

Merged
corentinravoux merged 30 commits intomainfrom
packaging_improvements
Jan 23, 2026
Merged

Packaging improvements#92
corentinravoux merged 30 commits intomainfrom
packaging_improvements

Conversation

@corentinravoux
Copy link
Owner

A lot of change to improve the readability of flip. For now it includes:

  • Linting error fix with Ruff
  • Docstring generation
  • Implementation of an help mode for each submodules

Planned change in this PR:

  • Package reformating: Implementation of different field level methods
  • Description of covariance models & testing the necessary input parameters
  • Improve the online documentation
  • Improve the tests and make the workflow in github

Copilot AI review requested due to automatic review settings December 3, 2025 13:13
Copy link

Copilot AI left a 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 pull request focuses on improving the code quality and documentation of the flip package through linting fixes, comprehensive docstring generation, and structural improvements.

Key Changes

  • Replaced bare except: with specific except ImportError: for better error handling
  • Renamed config.py to _config.py and updated all imports accordingly
  • Added comprehensive Google-style docstrings across all modules
  • Improved variable naming (e.g., lell for mathematical clarity)
  • Replaced eval() calls with safer importlib and getattr() patterns
  • Fixed type comparisons from type(x) == type(None) to x is not None

Reviewed changes

Copilot reviewed 34 out of 35 changed files in this pull request and generated 5 comments.

Show a summary per file
File Description
flip/utils.py Added docstrings for utility functions
flip/likelihood.py Import path fix + comprehensive class/method documentation
flip/power_spectra/*.py Exception handling improvements + docstrings
flip/covariance/*.py Import path updates, variable renaming (lell), docstrings
flip/fitter.py Reorganized imports + added docstrings
flip/data_vector/*.py Import path updates + docstrings
flip/_config.py New configuration file (renamed from config.py)
Comments suppressed due to low confidence (2)

flip/covariance/lai22/symbolic.py:143

    f = open(filename, "w")

flip/covariance/symbolic.py:201

    f = open(filename, "w")

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines 143 to 145
model_function = importlib.import_module(
f"flip.power_spectra.models.get_{power_spectrum_model}_model"
)
Copy link

Copilot AI Dec 3, 2025

Choose a reason for hiding this comment

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

Incorrect usage of importlib.import_module. This function imports a module, not a function. The code should be:

models = importlib.import_module(f"flip.power_spectra.models")
model_function = getattr(models, f"get_{power_spectrum_model}_model")

The current code will fail at runtime with an ImportError since flip.power_spectra.models.get_{power_spectrum_model}_model is not a module path.

Copilot uses AI. Check for mistakes.
@@ -147,6 +236,7 @@ def covariance_gg(
size_batch=100_000,
number_worker=8,
):
"""Compute density-density covariance (plane-parallel)."
Copy link

Copilot AI Dec 3, 2025

Choose a reason for hiding this comment

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

Missing closing quote in docstring. The line should end with:

    """Compute density-density covariance (plane-parallel)."""
Suggested change
"""Compute density-density covariance (plane-parallel)."
"""Compute density-density covariance (plane-parallel)."""

Copilot uses AI. Check for mistakes.
Comment on lines +481 to +487
"""Create an emcee sampler with optional HDF backend.

Args:
likelihood (Callable): Log-probability function.
p0 (numpy.ndarray, optional): Initial positions.
backend_file (str, optional): HDF backend filename to resume/checkpoint.
"""
Copy link

Copilot AI Dec 3, 2025

Choose a reason for hiding this comment

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

Misplaced docstring. The docstring should be inside the __init__ method, not after the super().__init__() call. Move lines 481-487 to be the first line after the function signature on line 479.

Copilot uses AI. Check for mistakes.
Comment on lines 240 to 261
"""Evaluate and optionally regularize M(k) term.

Applies one of the supported regularizations to stabilize numerical
behavior (mpmath high-precision, Savitzky–Golay smoothing, or low-k
asymptote detection).

Args:
M_function (callable): Function returning M(k) given additional parameters.
wavenumber (ndarray): k samples.
regularize_M_terms (dict|None): Per type regularization option.
covariance_type (str): `"gg"`, `"gv"`, or `"vv"`.
flip_terms (module): Terms module to switch backend when needed.
additional_parameters_values (dict|tuple): Extra parameters for M.
savgol_window (int): Window size for Savitzky–Golay.
savgol_polynomial (int): Polynomial order for Savitzky–Golay.
lowk_unstable_threshold (float): Threshold for low-k asymptote detection.
lowk_unstable_mean_filtering (int): Window for mean filtering indices.
mpmmath_decimal_precision (int): Decimal precision for mpmath.

Returns:
ndarray: Evaluated M(k) after regularization.
"""
Copy link

Copilot AI Dec 3, 2025

Choose a reason for hiding this comment

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

Misplaced docstring inside function body. The docstring for regularize_M should start on line 239, not line 240. Move the entire docstring block (lines 240-261) to immediately after line 239.

Suggested change
"""Evaluate and optionally regularize M(k) term.
Applies one of the supported regularizations to stabilize numerical
behavior (mpmath high-precision, SavitzkyGolay smoothing, or low-k
asymptote detection).
Args:
M_function (callable): Function returning M(k) given additional parameters.
wavenumber (ndarray): k samples.
regularize_M_terms (dict|None): Per type regularization option.
covariance_type (str): `"gg"`, `"gv"`, or `"vv"`.
flip_terms (module): Terms module to switch backend when needed.
additional_parameters_values (dict|tuple): Extra parameters for M.
savgol_window (int): Window size for SavitzkyGolay.
savgol_polynomial (int): Polynomial order for SavitzkyGolay.
lowk_unstable_threshold (float): Threshold for low-k asymptote detection.
lowk_unstable_mean_filtering (int): Window for mean filtering indices.
mpmmath_decimal_precision (int): Decimal precision for mpmath.
Returns:
ndarray: Evaluated M(k) after regularization.
"""
"""Evaluate and optionally regularize M(k) term.
Applies one of the supported regularizations to stabilize numerical
behavior (mpmath high-precision, SavitzkyGolay smoothing, or low-k
asymptote detection).
Args:
M_function (callable): Function returning M(k) given additional parameters.
wavenumber (ndarray): k samples.
regularize_M_terms (dict|None): Per type regularization option.
covariance_type (str): `"gg"`, `"gv"`, or `"vv"`.
flip_terms (module): Terms module to switch backend when needed.
additional_parameters_values (dict|tuple): Extra parameters for M.
savgol_window (int): Window size for SavitzkyGolay.
savgol_polynomial (int): Polynomial order for SavitzkyGolay.
lowk_unstable_threshold (float): Threshold for low-k asymptote detection.
lowk_unstable_mean_filtering (int): Window for mean filtering indices.
mpmmath_decimal_precision (int): Decimal precision for mpmath.
Returns:
ndarray: Evaluated M(k) after regularization.
"""

Copilot uses AI. Check for mistakes.
@@ -331,7 +299,7 @@ def run(self, migrad=True, hesse=False, minos=False, n_iter=1):
if minos:
try:
log.add(self.minuit.minos())
except:
except RuntimeError:
Copy link

Copilot AI Dec 3, 2025

Choose a reason for hiding this comment

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

'except' clause does nothing but pass and there is no explanatory comment.

Suggested change
except RuntimeError:
except RuntimeError:
# minuit.minos() may fail for some parameter configurations; ignore and proceed.

Copilot uses AI. Check for mistakes.
@corentinravoux
Copy link
Owner Author

Rather simple tests are working now

@corentinravoux
Copy link
Owner Author

Tests improved and test pipeline working. Adding a script to refresh the test values

@corentinravoux
Copy link
Owner Author

Fisher seems broken (blocked) with JAX at the moment

@corentinravoux
Copy link
Owner Author

New changes:

  • Package reformating: Possibility to create a framework for SBI or forward modeling
    
  • Improve the tests and make the workflow in github
    

Changes for future PR:

  • Description of covariance models & testing the necessary input parameters
    
  • Improve the online documentation
    

@corentinravoux corentinravoux linked an issue Jan 23, 2026 that may be closed by this pull request
@corentinravoux
Copy link
Owner Author

corentinravoux commented Jan 23, 2026

According to the extension of package,

  • Data Vector specific for Fisher were removed
  • The computation of covariance directly from data vectors is still possible, but not the default

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 76 out of 119 changed files in this pull request and generated 2 comments.

Comments suppressed due to low confidence (3)

flip/covariance/fitter.py:304


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

from flip import utils

(kmm, pmm), (kmt, pmt), (ktt, ptt) = load_power_spectra()
power_spectrum_dict = {
Copy link

Copilot AI Jan 23, 2026

Choose a reason for hiding this comment

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

This assignment to 'power_spectrum_dict' is unnecessary as it is redefined before this value is used.

Copilot uses AI. Check for mistakes.
return self._data

@abc.abstractmethod
def give_data_and_variance(self, **kwargs):
Copy link

Copilot AI Jan 23, 2026

Choose a reason for hiding this comment

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

Overridden method signature does not match call, where it is passed too many arguments. Overriding method method Dens.give_data_and_variance matches the call.
Overridden method signature does not match call, where it is passed too many arguments. Overriding method method DirectVel.give_data_and_variance matches the call.
Overridden method signature does not match call, where it is passed too many arguments. Overriding method method DirectVel.give_data_and_variance matches the call.
Overridden method signature does not match call, where it is passed too many arguments. Overriding method method Dens.give_data_and_variance matches the call.
Overridden method signature does not match call, where it is passed too many arguments. Overriding method method DirectVel.give_data_and_variance matches the call.
Overridden method signature does not match call, where it is passed too many arguments. Overriding method method DensVel.give_data_and_variance matches the call.

Copilot uses AI. Check for mistakes.
@corentinravoux
Copy link
Owner Author

This branch is major and will be the basis for v1.2 once the versioning pipeline works

@corentinravoux corentinravoux merged commit 2b140ef into main Jan 23, 2026
7 checks passed
@corentinravoux corentinravoux deleted the packaging_improvements branch January 23, 2026 14:28
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 76 out of 119 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (3)

flip/covariance/fitter.py:304


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

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.

Expand to include forward modeling methods

1 participant