Address PR review comments: improve configurability, error handling, and testing

[Copilot]

This PR addresses all 11 comments from the code review to improve code quality, robustness, and testability across the planner module.

Key Changes

Enhanced Configurability

Previously hardcoded values are now configurable with sensible defaults:

• Q resolution scaling in perform_mcmc() is now configurable (default: 0.025)
• Error parameters in add_instrumental_noise() are now configurable (min: 0.01, base: 0.05)

Improved Error Handling and Safety

• KDE error handling: Added proper logging with exc_info=True and regularized fallback to MVN entropy calculation
• Division by zero protection: Safe handling when np.max(q_values) is zero
• Module loading: Replaced unsafe sys.path injection with importlib.util.spec_from_file_location() for cleaner, safer module imports

Comprehensive Testing Infrastructure

Converted test_planner.py from a simple script with print statements to a proper pytest test suite:

• 16 new test cases across 3 test classes with proper assertions
• Edge case testing: Empty parameter lists, zero realizations, invalid parameters
• Error handling validation: Tests for non-existent files, mismatched arrays, invalid parameter names
• Integration testing: Ensures all components work together correctly

Code Quality Improvements

• Applied named expressions and simplified length comparisons
• Inlined immediately returned variables
• Fixed undefined variable reference in parameter validation

Example Usage

The new configurable parameters allow for more flexible experimental setups:

# Custom Q resolution scaling
samples = designer.perform_mcmc(q_values, noisy_r, errors, q_resolution_scale=0.05)

# Custom noise model parameters  
noisy_r, errors = add_instrumental_noise(
    q_values, r_calc, counting_time,
    min_relative_error=0.02,  # 2% minimum error
    base_relative_error=0.08   # 8% base error
)

Quality Assurance

• All existing tests continue to pass (60 tests)
• 16 new comprehensive tests added
• No breaking changes to existing APIs
• Enhanced logging provides better debugging information
• Integration testing confirms all changes work seamlessly together

This PR makes the codebase more maintainable, testable, and robust while preserving all existing functionality.

Original prompt

---

This section details on the original issue you should resolve

<issue_title>PR review</issue_title>
<issue_description>Please address the comments from this code review:

Overall Comments

• In expt_from_model_file, avoid injecting into sys.path—use importlib.util.spec_from_file_location to load the module directly by path, which is safer and doesn’t pollute the import namespace.
• The ExperimentDesigner.optimize (and related methods) use print statements for progress and results—consider switching to the logger or exposing a verbosity flag so output can be captured or silenced consistently.
• test_planner.py has no assertions and relies on random noise—add proper assert-based checks and seed the RNG (or allow injecting a seed) to make tests deterministic and verifiable.

Individual Comments

Comment 1

analyzer_tools/planner/experiment_design.py:204-205
<code_context>

•    """
  

•    # Prepare FitProblem
  

•    probe = QProbe(q_values, q_values * 0.025, R=noisy_reflectivity, dR=errors)
  

•    expt = Experiment(sample=self.experiment.sample, probe=probe)
  

•    problem = FitProblem(expt)
  

•    problem.model_update()
  

</code_context>

<issue_to_address>
suggestion: Hardcoded Q resolution scaling may not generalize to all instruments.

Consider making the Q resolution scaling factor configurable to accommodate different experimental setups.

Suggested implementation:

        """
        # Prepare FitProblem

        probe = QProbe(q_values, q_values * q_resolution_scale, R=noisy_reflectivity, dR=errors)
        expt = Experiment(sample=self.experiment.sample, probe=probe)
        problem = FitProblem(expt)
        problem.model_update()

            errors: Error bars
            mcmc_steps: Number of MCMC steps
            burn_steps: Number of burn-in steps
            q_resolution_scale: Scaling factor for Q resolution (default: 0.025)

        Returns:
            Tuple of (2D numpy array of MCMC samples, list of parameter names)
        """

def your_function_name(self, q_values, noisy_reflectivity, errors, mcmc_steps, burn_steps, q_resolution_scale=0.025):

• Replace your_function_name with the actual function name in your code.
• Update all calls to this function elsewhere in your codebase to pass the appropriate q_resolution_scale if a value other than the default is needed.
  </issue_to_address>

Comment 2

analyzer_tools/planner/experiment_design.py:297-306
<code_context>

• def _calculate_posterior_entropy_kdn(self, mcmc_samples: np.ndarray) -> float:
  </code_context>

<issue_to_address>
suggestion (bug_risk): KDE fallback to MVN may mask underlying issues.

Consider logging exception details or improving error handling to ensure issues with KDE or sample data are visible.

Suggested implementation:

    import logging

    def _calculate_posterior_entropy_kdn(self, mcmc_samples: np.ndarray) -> float:
        """
        Calculate posterior entropy using kernel density estimation.

        Args:
            mcmc_samples: 2D array of MCMC samples

        Returns:
            Posterior entropy in bits
        """
        if mcmc_samples.ndim != 2 or mcmc_samples.shape[0] < 2:

        try:
            kde = gaussian_kde(mcmc_samples.T)
            log_probs = kde.logpdf(mcmc_samples.T)
            entropy_nats = -np.mean(log_probs)
            return entropy_nats / np.log(2)
        except Exception as e:
            # Fallback to multivariate normal entropy if KDE fails
            logging.error("KDE failed in _calculate_posterior_entropy_kdn: %s", str(e), exc_info=True)
            print("Warning: KDE failed, falling back to MVN entropy. Exception: {}".format(e))
            cov_matrix = np.cov(mcmc_samples, rowvar=False)
            cov_matrix += 1e-10 * np.eye(cov_matrix.shape[0])  # Regularize
            entropy_nats = multivariate_normal.entropy(cov=cov_matrix)
            return entropy_nats / np.log(2)

</issue_to_address>

Comment 3

analyzer_tools/planner/instrument.py:23-24
<code_context>

•    Tuple of (noisy_reflectivity, errors)
  

• """

• Simple noise model: relative error inversely proportional to sqrt(R * counting_time)

• min_relative_error = 0.01 # 1% minimum
• base_relative_error = 0.05 # 5% base error

• Calculate relative errors - more realistic model

</code_context>

<issue_to_address>
suggestion: Hardcoded error parameters may limit flexibility.

Consider making min_relative_error and base_relative_error configurable via function arguments to support varying instrument requirements.

Suggested implementation:

    Args:
        q_values: Q values
        reflectivity_curve: Noise-free reflectivity
        counting_time: Relative counting time (affects noise level)
        min_relative_error: Minimum relative error (default 0.01)
    ...

</details>
Fixes mdoucet/analyzer#3

<!-- START COPILOT CODING AGENT TIPS -->
---

💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more [Copilot coding agent tips](https://gh.io/copilot-coding-agent-tips) in the docs.