Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

QuasarNET afterburner only uses HIZ template results in overlap region #2299

Closed
dylanagreen opened this issue Jul 19, 2024 · 3 comments
Closed
Assignees

Comments

@dylanagreen
Copy link
Contributor

When rerunning redrock the qos-qn afterburner runs the HIZ and LOZ templates separately due to the way RR accepts priors. The intention in the code is to then compare the two results in the overlap region, and keep the one with the lower chi^2 (this is normally done internally in redrock).

When running the two templates separately the afterburner initializes global variables:

redshift, err_redshift, chi2, coeffs = np.zeros(targetid.size), np.zeros(targetid.size), np.inf * np.ones(targetid.size), np.zeros((targetid.size, 10))

When aggregating the output the results are compared to the global variables, and if the fit has a better chi^2 than the globally stored chi^2 variable the fit is accepted and updated in the global variables:

# aggregate the result:
best_chi2 = np.zeros(targetid.size, dtype='bool')
best_chi2_tmp = chi2[sel] > chi2_tmp
best_chi2[sel] = best_chi2_tmp
redshift[best_chi2], err_redshift[best_chi2], coeffs[best_chi2] = redshift_tmp[best_chi2_tmp], err_redshift_tmp[best_chi2_tmp], coeffs_tmp[best_chi2_tmp]

However, the loop neglects to update the global chi2 variable, so line 253 above is always comparing against np.inf * np.ones(targetid.size) as initialized in line 198. The effect of this is that since the HIZ templates are run after the LOZ templates the HIZ results will always overwrite the LOZ results in the overlap region, regardless of whether the fit is better or at a different redshift than the LOZ templates.

This is probably a one line fix, adding

chi2[best_chi2] = chi2_tmp[best_chi2_tmp]

to update the global chi2 variable. after L256 should solve this issue, although validating this might take a little bit of effort. Reminder that the overlap region is 1.4 < z < 1.6.

@sbailey
Copy link
Contributor

sbailey commented Jul 19, 2024

Good catch. An arguably better long term solution is to update rrdesi --templates to accept more than one template. Then the afterburner can call Redrock once, let Redrock sort out which HIZ/LOZ is best by its standard algorithm, and then the after burner just uses that. It feels like a lot of the complication / fragility here is that rrdesi currently does not allow you to specify more than one template unless you give it a full directory worth of templates (which then brings in templates that the afterburner doesn't want to use).

@abrodze
Copy link
Member

abrodze commented Aug 28, 2024

Stephen's suggestion should already be possible since Redrock can take a text file for the --templates argument.

https://github.com/desihub/redrock/blob/main/py/redrock/templates.py#L298-307

I'll play around with the qnqso.py script to get it up to speed with this "new" redrock feature.

@sbailey
Copy link
Contributor

sbailey commented Nov 7, 2024

Fixed by PR #2402; closing

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

No branches or pull requests

3 participants