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

[bump minor] Cross exposure #1056

Merged
merged 76 commits into from
Jun 12, 2024
Merged

[bump minor] Cross exposure #1056

merged 76 commits into from
Jun 12, 2024

Conversation

corentinravoux
Copy link
Contributor

Large modifications of the pk1d routines, and some modifications at the delta extraction. The main point of the PR is to implement an estimator of the one-dimensional power spectrum based on cross-correlations between different exposures of the same quasar. This PR can be extended to the computation of P3D (P_cross in particular).

For P3D: Just use picca_Pk1d.py, and the new individual pk1d file contains the deltas' Fourier transforms.
For P1D cross-exposures: first use picca_delta_extraction.py with the options (use non-coadded spectra = True and keep single exposures = True) in [data], and calculate the Fourier transforms of the deltas with picca_Pk1d.py. Then, I've added a picca_Pk1D_cross_exposure.py script that estimates individual P1D cross-exposures.

The hope of this PR is to have a proper P1D cross-exposure estimator, and to start the process of implementing P3D estimators directly inside picca.

@corentinravoux
Copy link
Contributor Author

I am currently testing on iron: I made one run which worked with best exposure CF, and I am running separated exposure CF to compare.
Once we decide the best way to perform CF, we can implement the test on delta extraction, and maybe finally close this PR.

@corentinravoux
Copy link
Contributor Author

Copying slack message without plots, as it is private data:

I have computed P1D on Y1 with two ways to measure the CF:
CF on the best exposure, applied to all exposures
CF on all exposures independently
The result for independent exposures is almost perfect, no deviation to Y1 in the statistical errors bars of Y1 which are really low. And this without any modeling of the noise. With this result, I am starting wondering if it is necessary to look for correlated noise sources.
The best exposure case result is not that good. There seems to be an outlier in the z=2.4 bin. But even without this one, there is a 3/4% shift for almost all scales. To understand this shift I checked the continuum of a specific quasar which have 2 independent exposures. The continuum is shown on the third plot for each exposure and for the coadd (realized in the previous runs of Y1). It seems like computing the delta of an exposure with the continuum computed on another one (either the best SNR exposure but I think also the coadd) will bias the deltas and consequently the P1D.
Here, the stack of delta is not forced to zero, this solution could also help to reduce this bias, but my main conclusion is that treating exposures independently is enough.

@corentinravoux
Copy link
Contributor Author

Since the independent exposure continuum fitting case works very well, I think I will let this solution for the PR, and remove the others solutions.
@Waelthus What do you think about that ?

@Waelthus
Copy link
Contributor

I think given the biases the "best exp" case might not be a good one to keep and having the 2 options of either using individual exposures for cont fitting (thus neglecting that the true QSO continuum should be ~ the same (although when there's long timelags between the obs it can also be the quasar changed a bit)) and co-adding (which I believe you've sped up).
Having 2 alternative options here should be enough to allow reproducing that test later on, e.g. when writing the paper or dealing with ref comments... Agreed that the main analysis should probably be run on the indiv exp case if that's the only relatively unbiased estimate...
Did you add options for selecting different exp only if they are done on different tiles as well etc? Such that teff is basically nominal for all exps (instead of having e.g. split exps as different exposures, or aren't they?)

@corentinravoux
Copy link
Contributor Author

corentinravoux commented Apr 30, 2024

Ok, so for the last step of this PR, I will try to had an option which defines how the CF is performed, and it will also allows to separete the standard case to the cross-exposure case.
For your last question, this is an interesting point. For now I am working on the healpix, and after checking on the spectra directory, a very large portion of the exposures are on the same tile (which is weird by the way). So, I do not think selecting on this criteria would be optimal.
Another option might be to cut on the SNR scores in the spectra ? to remove exposures with EFFTIME lower than the nominal time ?

@corentinravoux
Copy link
Contributor Author

After consultation with the desi data team, the best solution to get rid of both splitted exposures and correlated noise might be to keep only one repetition of TARGETID+NIGHT. I do not know if we add it to this PR.

@Waelthus
Copy link
Contributor

Waelthus commented May 2, 2024

What does this mean in here? Taking only one exp or rather taking a coadd of all same-TARGETID exposures in the same night, but not coadding across nights?

@corentinravoux
Copy link
Contributor Author

I meant taking only one exposure. I think adding a coaddition step during the night might be too complicated, and I do not think we will gain much from it.

@Waelthus
Copy link
Contributor

Waelthus commented May 6, 2024

I guess it could be ok to only take the best of the split exposures in that case (I guess typical splits have most of the SNR in the first spectrum [conditions worsen while observing leading to an early stopping] OR have about equal T_eff when conditions were not great, but good enough for splits), but that would have less SNR than usual... Depending on how often exposures are actually split one could also think about removing splits entirely (that would keep the sample uniform, but we'd lose more spectra)...

@Waelthus Waelthus changed the title Cross exposure [bump minor] Cross exposure May 6, 2024
@corentinravoux
Copy link
Contributor Author

With this new commit, I have put all the options we wanted.
We can focus on the testing and merge now.

@corentinravoux
Copy link
Contributor Author

Hi @Waelthus, Thanks a lot for all those fix !
I am happy with the state of the PR, do you still have in mind unresolved issues or potential options to implement ?

Copy link
Contributor

@Waelthus Waelthus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, I had a quick last look and think this is ok. If @iprafols is fine as well we could merge.
Basic tests are included, but end-to-end is still missing afaik. Should keep this in mind for future development.
Potentially should add the new parameters etc to the relevant drawings etc...

@@ -37,6 +37,7 @@
"rejection log file",
"save format",
"num processors",
"delta extraction single exposure",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not so sure this name is really clear, potentially should add something to the parameter descriptions in the readme-files or have a more self-explanatory name. Maybe something like "spectral coadding method", but given that this is only applied for cont fitting might be misleading... @corentinravoux @iprafols any ideas?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that as long as we explain this somewhere this name should be fine

@iprafols
Copy link
Collaborator

iprafols commented Jun 3, 2024

I am fine with merging

@Waelthus Waelthus added this pull request to the merge queue Jun 12, 2024
Merged via the queue into master with commit 5d5a0a7 Jun 12, 2024
10 checks passed
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.

3 participants