From 90c2c21ba932a5227b0c880463d5e9a75fc88c27 Mon Sep 17 00:00:00 2001
From: "Christopher H. Jordan" <christopherjordan87@gmail.com>
Date: Wed, 6 Apr 2022 16:51:44 +0800
Subject: [PATCH] Fix issues around averaging factors.

The parse_{time,freq}_average_factor functions expect {time,freq}
resolutions, but they weren't getting them, and things weren't working
as expected. This commit fixes that and an associated test.
---
 src/calibrate/params/mod.rs   | 70 ++++++++++++++++-------------------
 src/calibrate/params/tests.rs | 12 ++++--
 2 files changed, 40 insertions(+), 42 deletions(-)

diff --git a/src/calibrate/params/mod.rs b/src/calibrate/params/mod.rs
index 167b27d8..e073641a 100644
--- a/src/calibrate/params/mod.rs
+++ b/src/calibrate/params/mod.rs
@@ -804,44 +804,38 @@ impl CalibrateParams {
             } else {
                 // Parse and verify user input (specified resolutions must
                 // evenly divide the input data's resolutions).
-                let time_factor = parse_time_average_factor(
-                    obs_context
-                        .time_res
-                        .map(|res| res * time_average_factor as f64),
-                    output_vis_time_average,
-                    1,
-                )
-                .map_err(|e| match e {
-                    AverageFactorError::Zero => InvalidArgsError::OutputVisTimeAverageFactorZero,
-                    AverageFactorError::NotInteger => {
-                        InvalidArgsError::OutputVisTimeFactorNotInteger
-                    }
-                    AverageFactorError::NotIntegerMultiple { out, inp } => {
-                        InvalidArgsError::OutputVisTimeResNotMulitple { out, inp }
-                    }
-                    AverageFactorError::Parse(e) => {
-                        InvalidArgsError::ParseOutputVisTimeAverageFactor(e)
-                    }
-                })?;
-                let freq_factor = parse_freq_average_factor(
-                    obs_context
-                        .freq_res
-                        .map(|res| res * freq_average_factor as f64),
-                    output_vis_freq_average,
-                    1,
-                )
-                .map_err(|e| match e {
-                    AverageFactorError::Zero => InvalidArgsError::OutputVisFreqAverageFactorZero,
-                    AverageFactorError::NotInteger => {
-                        InvalidArgsError::OutputVisFreqFactorNotInteger
-                    }
-                    AverageFactorError::NotIntegerMultiple { out, inp } => {
-                        InvalidArgsError::OutputVisFreqResNotMulitple { out, inp }
-                    }
-                    AverageFactorError::Parse(e) => {
-                        InvalidArgsError::ParseOutputVisFreqAverageFactor(e)
-                    }
-                })?;
+                let time_factor =
+                    parse_time_average_factor(obs_context.time_res, output_vis_time_average, 1)
+                        .map_err(|e| match e {
+                            AverageFactorError::Zero => {
+                                InvalidArgsError::OutputVisTimeAverageFactorZero
+                            }
+                            AverageFactorError::NotInteger => {
+                                InvalidArgsError::OutputVisTimeFactorNotInteger
+                            }
+                            AverageFactorError::NotIntegerMultiple { out, inp } => {
+                                InvalidArgsError::OutputVisTimeResNotMulitple { out, inp }
+                            }
+                            AverageFactorError::Parse(e) => {
+                                InvalidArgsError::ParseOutputVisTimeAverageFactor(e)
+                            }
+                        })?;
+                let freq_factor =
+                    parse_freq_average_factor(obs_context.freq_res, output_vis_freq_average, 1)
+                        .map_err(|e| match e {
+                            AverageFactorError::Zero => {
+                                InvalidArgsError::OutputVisFreqAverageFactorZero
+                            }
+                            AverageFactorError::NotInteger => {
+                                InvalidArgsError::OutputVisFreqFactorNotInteger
+                            }
+                            AverageFactorError::NotIntegerMultiple { out, inp } => {
+                                InvalidArgsError::OutputVisFreqResNotMulitple { out, inp }
+                            }
+                            AverageFactorError::Parse(e) => {
+                                InvalidArgsError::ParseOutputVisFreqAverageFactor(e)
+                            }
+                        })?;
 
                 (time_factor, freq_factor)
             };
diff --git a/src/calibrate/params/tests.rs b/src/calibrate/params/tests.rs
index 4c31e533..987784ce 100644
--- a/src/calibrate/params/tests.rs
+++ b/src/calibrate/params/tests.rs
@@ -383,16 +383,20 @@ fn test_handle_bad_array_pos() {
 }
 
 #[test]
-/// test that it correctly stacks averaging factor.
+/// Test that the input visibility frequency average factor stacks with the
+/// output frequency averaging factor.
 ///
-/// TODO(dev): use test data with multiple timesteps to test time factor stacking
+/// TODO(dev): use test data with multiple timesteps to test time factor
+/// stacking
 fn test_handle_both_vis_avg() {
     let mut args = get_reduced_1090008640(true);
+    // The input data is 40kHz; input is averaged 2x
     args.freq_average_factor = Some("80kHz".into());
+    // Output is averaged 2x from the averaged input; a total of 4.
     args.output_vis_freq_average = Some("160kHz".into());
     args.outputs = Some(vec!["test.uvfits".into()]);
     let result = args.into_params().unwrap();
 
-    // output averaging factor should be relative to cal averaging factor.
-    assert_eq!(result.output_vis_freq_average_factor, 2);
+    // output averaging factor is relative to the input frequency resolution.
+    assert_eq!(result.output_vis_freq_average_factor, 4);
 }