igmhub / picca

set of tools for continuum fitting, correlation function calculation, cosmological fits...
GNU General Public License v3.0
29 stars 22 forks source link

fixed issue 993 #994

Closed iprafols closed 1 year ago

iprafols commented 1 year ago

Here I changed the method _initialize_variance_wavelength_array from ExpectedFlux as suggested in issue #993.

The impact of this change is that the functions eta, var_lss (and fudge) are computed in a slightly different wavelength grid. The new grid selects points from Forest.log_lambda_grid thus enforcing the actual wavelength cuts, whereas before we were having an array that was mildly out of bounds. This is an issue if we want to use direct computation in the function find_bins.

As expected, I also needed to update the test suite. However, given that the fits to compute eta, var_ls, and fudge in the test do not converge (since we do not reach the minimum number of quasars), the changes are limited to the remapping of the wavelength grid for the VAR_STATS HDU in the different files.

The exception to this is the file true_iter_out_prefix_compute_expected_flux_log.fits.gz. For this file, the change in var_lss is larger than expected (see red vs blue line in the plot). I investigated the origin of this change and I found out that in the TrueContinuum class, sigma_lss is computed in the Forest.log_lambda_grid grid and then interpolated to the self.log_lambda_var_func_grid grid before saving. This "native" grid is shown as the orange line in the plot. The change observed in the VAR_STATS HDU is associated with the noisy nature of this function. This might be an issue for the runs using TrueContinuum expected flux but is outside the scope of this PR (I will now file this as an issue).

download-1

Because the fits to compute eta, var_ls, and fudge in the test do not converge, the impact of this change in the final deltas is not assessed by the automatic tests. I expect this change to be small, but I would like to verify this on a fugu run before merging this branch

iprafols commented 1 year ago

As expected the changes are small:

changes

I think we are now ready to merge

iprafols commented 1 year ago

Replaced by #1000