Skip to content

[WIP] Complete extensive code refactoring of scintools#30

Closed
Copilot wants to merge 1 commit intocopilot/refactor-scintools-codefrom
copilot/complete-code-refactoring-scintools
Closed

[WIP] Complete extensive code refactoring of scintools#30
Copilot wants to merge 1 commit intocopilot/refactor-scintools-codefrom
copilot/complete-code-refactoring-scintools

Conversation

Copy link

Copilot AI commented Mar 3, 2026

Thanks for asking me to work on this. I will get started on it and keep this PR's description up to date as I form a plan and make progress.

Original prompt

Complete Extensive Code Refactoring of scintools

Overview

This PR builds on PR #29 to complete the extensive code refactoring that was deferred. The focus is on modernizing code patterns, improving architecture, and enhancing maintainability while maintaining backward compatibility.

Branch to Work From

Base this PR on the branch copilot/refactor-scintools-code from PR #29, NOT on master. This ensures we build on the documentation improvements already made.

Primary Focus: dynspec.py Refactoring

1. Extract Repeated Utility Functions to scint_utils.py

Identify and extract commonly used patterns that appear multiple times in dynspec.py:

Array Validation & Cleaning:

  • Functions that check for NaN/inf values
  • Functions that validate array shapes
  • Functions that handle masked arrays
  • Functions that normalize/rescale data

Example patterns to extract:

# Pattern: Validating frequency/time arrays
def validate_freq_array(freqs, bw, nchan):
    """Validate frequency array has correct shape and values."""
    
# Pattern: Handling NaN values in 2D arrays
def clean_2d_array(arr, method='interpolate'):
    """Clean 2D array by handling NaN/inf values."""

# Pattern: Creating frequency/time grids
def create_freq_grid(freq, bw, nchan):
    """Create frequency grid for dynamic spectrum."""

Look specifically for:

  • Repeated array shape validation
  • Repeated NaN/inf handling
  • Repeated normalization calculations
  • Repeated frequency/time grid creation
  • Repeated plotting setup code

2. Modernize Code Patterns Throughout

A. Replace .format() with f-strings:

Current pattern:

print("Now adding {} ...".format(other.name))
print("WARNING: Code does not yet account for different frequency properties")

Modern pattern:

print(f"Now adding {other.name}...")
print(f"WARNING: Code does not yet account for different frequency properties")

Scan ALL print statements, error messages, and string formatting throughout dynspec.py and replace with f-strings.

B. Add Type Hints:

Add type hints to all method signatures in the Dynspec class:

from typing import Optional, Union, Tuple, List
from pathlib import Path
import numpy as np

def __init__(
    self, 
    filename: Optional[Union[str, Path]] = None,
    dyn: Optional['Dynspec'] = None,
    verbose: bool = True,
    process: bool = False,
    lamsteps: bool = False,
    remove_short_subs: bool = True,
    subint_thresh: float = 2.33,
    mjd: Optional[float] = None
) -> None:

Add type hints to:

  • All __init__ and class methods
  • Return types
  • All new utility functions
  • Complex data structures (use TypedDict or dataclasses where appropriate)

C. Use pathlib for File Operations:

Current pattern:

from os.path import split
archive_path, archive_name = os.path.split(archive.get_filename())

Modern pattern:

from pathlib import Path
archive_path = Path(archive.get_filename())
archive_name = archive_path.name

Replace all:

  • os.path.join()Path() / filename
  • os.path.split()Path().parent and Path().name
  • String path manipulations → Path methods
  • File existence checks → Path().exists()

D. Improve Error Handling:

Replace generic print statements with proper exceptions:

Current pattern:

print("Error: No dynamic spectrum file or object")

Modern pattern:

raise ValueError("No dynamic spectrum file or object provided. "
                "Specify either 'filename' or 'dyn' parameter.")

Add specific exceptions:

  • ValueError for invalid parameter values
  • FileNotFoundError for missing files
  • TypeError for wrong types
  • Custom exceptions if needed (e.g., DynspecError)

3. Refactor fit_arc() Method

This is the HIGHEST PRIORITY item. The fit_arc method has 24 parameters and is very complex.

A. Create Configuration Classes:

Use dataclasses to group related parameters:

from dataclasses import dataclass, field
from typing import Optional, Tuple

@dataclass
class ArcFitConfig:
    """Configuration for arc curvature fitting."""
    
    # Curvature search parameters
    numsteps: int = 10000
    etamax: Optional[float] = None
    etamin: Optional[float] = None
    constraint: Tuple[float, float] = (0.0, np.inf)
    
    # Data preparation
    startbin: int = 3
    cutmid: int = 3
    delmax: Optional[float] = None
    lamsteps: bool = False
    
    # Fitting parameters
    low_power_diff: float = -1.0
    high_power_diff: float = -0.5
    nsmooth: int = 5
    log_parabola: bool = False
    logsteps: bool = False
    weighted: bool = False
    
    # Plotting
    plot: bool = False
    display: bool = True
    figsize: Tuple[int, int] = (9, 9)
    dpi: int = 200
    filename: Optional[str] = None
    figN: Optional[int] = None
    plot_spec: bool = False
    
    # Other
    ref_freq: float = 1400.0
    efac: float = 1.0
    noise_error: bool ...

</details>



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

*This pull request was created from Copilot chat.*
>

<!-- 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.

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.

2 participants