Commit fb35c70b authored by Gaetano Scandariato's avatar Gaetano Scandariato
Browse files

refactor of group 5 (configuration and parsing) completed.

Perform a full review and cleanup of the configuration parsing system to ensure correctness, determinism, and maintainability. This includes resolving all items listed in the Group 5 review plan.

- Bug Fixes:
    * Fourier Detrending: Fixed a bug in `detrending.py` where the sine coefficient's per-visit parameter set was silently dropped during hybrid accumulation.
    * Stellar Parameters: Fixed `star.py` to store the literal string 'v_sin_i' instead of the distribution tuple, and ensured scope keywords are not ignored in list-form specifications.
    * Logging: Corrected `log_func` calls in `star.py` to include the required `plModel` argument to prevent runtime TypeErrors.
    * Scoping: Removed unreachable code in `scoping.py` following a ValueError raise.
    * EIV Latent Variables: Fixed `detrending.py` to ensure `mu_xi` and `log_tau` correctly inherit scope from the `slope` parameter while allowing explicit user overrides.

- Architecture & Refactoring:
    * Circular Imports: Introduced `PyAETNA/sampling/config/parser/_constants.py` to centralize `_SCOPE_KEYWORDS` and `_PRIOR_REF_RE`, resolving circular dependencies between `scoping.py` and `prior_refs.py`.
    * Determinism: Implemented a canonical `param_order` in `parse_config` (Jitter -> Norm -> Detrend -> Star -> Planet) to ensure deterministic parameter ordering in chain files across Python versions.

- Documentation & Quality:
    * Added module-level docstrings to all parser modules and `priors.py`.
    * Improved `priors._parse_prior_string` error messages to explicitly list all ten supported distribution types.
    * Cleaned up stale comments and added rationale documentation in `scoping.py` and `prior_refs.py`.

- Testing:
    * Created `tests/test_config_parser_group5.py` with 53+ tests covering:
        - Scoped parameter parsing and expansion (global, per_instrument, per_visit).
        - Prior string parsing for all supported distributions.
        - Specific parser logic for Fourier, Polynomials, and Stellar configurations.
    * Created `tests/test_eiv_inheritance.py` to verify the latent variable scope inheritance and override logic.
parent 8214dc7d
Loading
Loading
Loading
Loading
Loading
+39 −2
Changes for PyAETNA/sampling/config/parser/__init__.py: 39 added lines, 2 removed lines.
Original line number Diff line number Diff line
@@ -235,6 +235,7 @@ def parse_config(config, plModel, plData, log_func=None):
    all_params = list(free_params.keys())
    norm_params = [p for p in all_params if p.startswith('norm')]
    jitter_params = [p for p in all_params if p.startswith('log_jitter')]
    
    try:
        def _norm_sort_key(p):
            for prefix in ('norm_v', 'norm_'):
@@ -257,8 +258,44 @@ def parse_config(config, plModel, plData, log_func=None):
    except Exception:
        norm_params_sorted = sorted(norm_params)
        jitter_params_sorted = sorted(jitter_params)
    other_params = [p for p in all_params if p not in norm_params_sorted and p not in jitter_params_sorted]
    param_order = jitter_params_sorted + norm_params_sorted + other_params

    # Separate 'other' parameters into categories for deterministic ordering:
    # Order: Detrending -> Star -> Planets
    detrend_others = []
    star_others = []
    planet_others = []
    
    # Define prefixes/keywords for categorization
    # Detrending: poly, fourier, eiv, gp, norm
    # Star: v_sin_i, rho_star, q1, q2
    # Planets: usually contain planet names or 'planet_'
    
    # We can use the param_groups to identify these
    detrend_bases = {'poly', 'fourier', 'eiv', 'gp', 'norm'}
    star_bases = {'v_sin_i', 'rho_star', 'q1', 'q2'}
    
    # Identify which params belong to which group
    # Note: we check if the param name starts with any of the bases or is in a group
    for p in all_params:
        if p in norm_params_sorted or p in jitter_params_sorted:
            continue
            
        # Check if it's a star parameter
        if any(p.startswith(base) for base in star_bases):
            star_others.append(p)
        # Check if it's a detrending parameter
        elif any(p.startswith(base) for base in detrend_bases):
            detrend_others.append(p)
        else:
            # Assume it's a planet parameter if it's not star or detrending
            planet_others.append(p)

    # Sort each group alphabetically for stability
    detrend_others.sort()
    star_others.sort()
    planet_others.sort()

    param_order = jitter_params_sorted + norm_params_sorted + detrend_others + star_others + planet_others

    # =============================================
    # PRECOMPUTED PARAMETER MASKS (JAX boolean arrays)
+13 −0
Changes for PyAETNA/sampling/config/parser/_constants.py: 13 added lines, 0 removed lines.
Original line number Diff line number Diff line
"""
_constants.py — Shared constants for PyAETNA configuration parsing.

This module holds constants used across multiple parser modules to avoid
circular imports.
"""
import re

# Keywords used to define the scope of a parameter (global, per-instrument, or per-visit).
_SCOPE_KEYWORDS = frozenset(('global', 'per_instrument', 'per_visit'))

# Regular expression to identify a named prior reference (e.g., '@my_prior').
_PRIOR_REF_RE = re.compile(r'^@([A-Za-z_][A-Za-z0-9_]*)$')
+19 −22
Changes for PyAETNA/sampling/config/parser/detrending.py: 19 added lines, 22 removed lines.
Original line number Diff line number Diff line
@@ -304,39 +304,36 @@ def parse_eiv_linear(detrending, plModel, plData, n_instruments, visit_ids, inst

        # Register latent variables as sampled parameters
        # These will be given data-driven priors in the model_fn
        for latent_base, default_prior in [
            (f'{eiv_name}_mu_xi', default_mu_xi),
            (f'{eiv_name}_log_tau', default_log_tau),
        for latent_base, default_prior, config_key in [
            (f'{eiv_name}_mu_xi', default_mu_xi, 'mu_xi'),
            (f'{eiv_name}_log_tau', default_log_tau, 'log_tau'),
        ]:
            # Check if user provided a prior for this latent variable
            key_name = latent_base.split('_')[-2] # 'mu_xi' or 'log_tau'
            user_provided = key_name in eiv_cfg
            latent_raw = eiv_cfg.get(key_name, default_prior)
            user_provided = config_key in eiv_cfg
            latent_raw = eiv_cfg.get(config_key, default_prior)
            
            latent_raw = resolve_config_value(
                latent_raw, named_priors, f'detrending.eiv_linear.{eiv_name}.{key_name}'
                latent_raw, named_priors, f'detrending.eiv_linear.{eiv_name}.{config_key}'
            )
            
            if user_provided:
                l_scope, l_entries = _parse_scoped_param(
                    latent_raw, ('global', 'per_instrument'),
                    len(target_insts), f'{eiv_name}.{config_key}'
                )
            else:
                # Inherit scope from slope if not explicitly provided by user
            if not user_provided and not isinstance(latent_raw, list):
                l_scope = slope_scope
                # Convert the prior to the expected format for _expand_scoped_param
                # If it was a tuple ('normal', 0, 1), it becomes [('normal', 0, 1)]
                l_entries = [latent_raw] if isinstance(latent_raw, tuple) else [latent_raw]
                if l_scope == 'per_instrument':
                    l_entries = l_entries * len(target_insts)
            elif not user_provided:
                # Using default_prior (which is a list), but we want to override its scope
                l_scope = slope_scope
                # The default_prior is [scope, dist, p1, p2], we only want [dist, p1, p2]
                l_entries = [default_prior[1:]]
                # We use the distribution part of the default_prior
                if isinstance(latent_raw, list) and len(latent_raw) > 0 and latent_raw[0] in ('global', 'per_instrument'):
                    l_entries = [latent_raw[1:]]
                else:
                    l_entries = [latent_raw]
                
                if l_scope == 'per_instrument':
                    l_entries = l_entries * len(target_insts)
            else:
                l_scope, l_entries = _parse_scoped_param(
                    latent_raw, ('global', 'per_instrument'),
                    len(target_insts), f'{eiv_name}.{key_name}'
                )


            l_names = _expand_scoped_param(
                l_scope, l_entries, latent_base,
+1 −6
Changes for PyAETNA/sampling/config/parser/prior_refs.py: 1 added line, 6 removed lines.
Original line number Diff line number Diff line
@@ -10,12 +10,7 @@ configuration value).
import re

from .. import priors


_PRIOR_REF_RE = re.compile(r'^@([A-Za-z_][A-Za-z0-9_]*)$')
# NOTE: _SCOPE_KEYWORDS is intentionally duplicated from scoping.py to avoid
# a circular import (scoping.py already imports from this module).
_SCOPE_KEYWORDS = frozenset(('global', 'per_instrument', 'per_visit'))
from ._constants import _PRIOR_REF_RE, _SCOPE_KEYWORDS


class PriorReferenceValue:
+1 −2
Changes for PyAETNA/sampling/config/parser/scoping.py: 1 added line, 2 removed lines.
Original line number Diff line number Diff line
@@ -10,8 +10,7 @@ import numpy as np
import jax.numpy as jnp
from .. import priors
from .prior_refs import is_resolved_prior_reference, unwrap_prior_reference

_SCOPE_KEYWORDS = frozenset(('global', 'per_instrument', 'per_visit'))
from ._constants import _SCOPE_KEYWORDS

class _PerVisitEntry:
    """Marker for per_visit sub-entries inside a per_instrument spec.
Loading