igmhub / picca

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

[bump major] Unblinding y1 #1054

Closed andreicuceu closed 6 months ago

andreicuceu commented 7 months ago

Removes the automatic blinding of Y1 correlations. Only picca_export is changed, with the rest of the pipeline unaffected.

Note that the blinding field in the header of exported correlations ('desi_y1') will remain, because we still need Vega to know about it for full-shape blinding.

iprafols commented 7 months ago

I modified the code so that we can run the entire pipeline with the unbllind flag set. I have also changed its default value to True. Otherwise, it looks fine

iprafols commented 7 months ago

@andreicuceu , let me know if you're happy with the changes and then I'll merge the PR

Waelthus commented 7 months ago

I added a bump major tag to make it very clear that important things changed.

andreicuceu commented 7 months ago

Hi Ignasi, does the unblindable flag mean that the DESI runs will potentially not have the blinding=desi_y1 field in their header? I would like to keep that around for all runs by default if possible. It would make a lot easier to manage blinding for other non-key projects.

iprafols commented 7 months ago

Hi Andrei, so do you want to always have the blinding flag all the way to picca_export? Or having it as the default choice would also make sense?

If you always want to have the blinding flag on, then I suggest we move the variable UNBLINDABLE_STRATEGIES to pica_export and then we don't check for this at the delta extraction level.

If you can accept the possibility of having the blinding flag off for unblinded data, then I can just change back the default value.

What do you think? Since this is a bigger change, maybe we can start by changing the default value of unblinding to False and then discuss this on a picca task force telecon (maybe next week?) What do you think?

alxogm commented 6 months ago

Hello, is there any blocking factor to merge this PR? I would like to have a tagged version to re-run the unblinded baseline analysis.

andreicuceu commented 6 months ago

Hi @iprafols, just got back to this.

Yes, I would like the blinding flag to always be there.

In truth, that is not really a blinding flag anymore. It is a field that tells us which part of the survey the data comes from (EDR, Y1, Y3, etc.). We then use this information to apply or not blinding. So it might make sense to even rename that field to something like latest_survey (to indicate which is the latest survey we have data from in the dataset being analyzed). However, this is a change that would break backwards compatibility and be much larger than I want this pull request to be. So I suggest we leave it as an issue for the future.

For now, I think it would make sense to just move the UNBLINDABLE_STRATEGIES to picca_export, as you suggest.

iprafols commented 6 months ago

Hi @andreicuceu, I moved the variable UNBLINDABLE_STRATEGIES to picca_export and removed the check from delta_extraction. Can you check that you are ok with the changes? From my side we are ready to merge

andreicuceu commented 6 months ago

Hi @iprafols, look good. I'm happy to merge.