CIRADA-Tools / RM-Tools

RM-synthesis, RM-clean and QU-fitting on polarised radio spectra
MIT License
42 stars 23 forks source link

rescale_I_model_3D: rescale_I_pixel uses a dictionary but util_misc.renormalize_StokesI_model expects "FitResult" class #131

Closed ErikOsinga closed 4 months ago

ErikOsinga commented 4 months ago

As title describes, the call to renormalize_StokesI_model is outdated inside RMtools3D/rescale_I_model_3D.py since it calls it using a dictionary called oldDict instead of the FitResult class

AlecThomson commented 4 months ago

The fix should be as simple as something like this:

def rescale_I_pixel(
        data: Tuple[np.ndarray, np.ndarray, np.ndarray, np.ndarray], 
        fit_function: str
    ) -> Tuple[np.ndarray, np.ndarray]:
    covar, coeff, old_freq, new_freq = data
    old_result = FitResult(
        params=coeff,
        fitStatus=np.nan,  # Placeholder
        chiSq=np.nan, # Placeholder
        chiSqRed=np.nan, # Placeholder
        AIC=np.nan, # Placeholder
        polyOrd=len(coeff) - 1,
        nIter=0, # Placeholder
        reference_frequency_Hz=old_freq,
        dof=np.nan, # Placeholder
        pcov=covar,
        perror=np.zeros_like(coeff), # Placeholder
        fit_function=fit_function,
    )
    new_fit_result: FitResult = renormalize_StokesI_model(old_result, new_freq)
    return new_fit_result.params, new_fit_result.perror

But I'm hesitant to action bigger changes here without some tests on this module. I'm not super close to this code - could @ErikOsinga or @Cameron-Van-Eck pass me some expected inputs/outputs and we can add a test suite?

AlecThomson commented 4 months ago

Whilst I'm in here I'd recommend a couple of other changes - happy to get thoughts. I think the CLI logic and the main flow should be separated into the existing command_line() func and new main(...).

I'd also recommend we use the standard library pathlib.Path tooling for handling paths (over os.path).

The speed of light look a bit off! We should really just import it everywhere from astropy.constants and be done with it. It's a little crazy to type it out in multiple places.

As an aside, the choice to produce files like: https://github.com/CIRADA-Tools/RM-Tools/blob/885f88112e004f7b2cd151b9143a54dbe9dc28c2/RMtools_3D/do_fitIcube.py#L615

Makes it annoying to parse the filenames. I'd recommend we change the standard to something like:

outname = os.path.join(outDir, prefixOut + ".covariance.fits")

Although checking filenames is aways going to be a little yucky, I suppose.